The g_base64_decode function will return a valid pointer, but with
length of zero when it fails to decode data. Report an error in that
scenario, so avoid a later more obsecure error.
eg old behaviour
# virsh secret-set-value f52a81b2-424e-490c-823d-6bd4235bc507 foo
warning: Passing secret value as command-line argument is insecure!
error: Failed to set secret value
error: value in virSecretSetValue must not be NULL
new behaviour
# /home/berrange/virsh secret-set-value f52a81b2-424e-490c-823d-6bd4235bc507 foo
warning: Passing secret value as command-line argument is insecure!
error: Secret value is not valid base64
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The UUID had only 11 characters in the last component, instead of the
required 12. Add the missing trailing '2' which is present in all the
other matching example UUIDs.
Reviewed-by: Arun Menon <armenon@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Add an example of 250 layer deep nested JSON to make sure the parser
supports it. This is in order to maintain compatibility with external
snapshots in qemu, where such a deeply nested document is returned with
a 'query-block' QMP call.
I've used a fake JSON as a real reply from qemu is around 1.4MiB for a
200 deep image chain.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The default in json-c is 32 which is too low to accomodate the 200
snapshot layers we supported historically in the qemu driver (200 is
picked based on the 256 layer limit in libxml).
The response to 'query-block' is otherwise too low and we fail to start
the VM when there's around 26 images in a backing chain.
'json_tokener_new_ex' is supported since json-c 0.11 and we require at
least 0.14.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Currently, to probe PCI devices, for each device libvirt
calls "bhyve -s 0,<device_type>" and parses the error message to check
if this specific device is supported.
For quite some time, bhyve reports the list of devices using:
bhyve -s help
where it prints all supported devices, one device per line.
Update the code to use this command:
* It is more accurate as we don't need to rely on the error message
parsing.
* It's faster as we get all the devices in one run instead of
running bhyve for every device type.
* The code is simpler.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This partially reverts commit 3caa28dc50 which changed the DST times
to start at Jan 2 due to an issue that is already solved in GLib that we
require for almost a year now.
This also fixes the test failures that would go away on Jan 2 11:00
UTC (less than two hours from writing this commit message) due to the
timezone definitions (the last failing test case has a timezone which is
UTC+11:00), but because of the change from '0/' to '1/' it did occur on
a day which is not caught by isNearYearEnd() which exists solely for
this purpose.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
In the <dns/> section of network configuration users can set up
forwarding of DNS requests to custom DNS servers. These are
specified using 'addr' attribute. But configuring port wasn't
possible, until now. New 'port' attribute is introduced, which
allows overriding the default DNS port for given address.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Bhyve supports SLIRP networking using the following syntax:
-s 12:0,e1000,slirp,mac=<macaddr>,open
Where "e1000" is a NIC model, "slirp" is the SLIRP backend and "open"
specifies open mode where external network is available to the guest.
The "open" mode is a recent addition in FreeBSD -CURRENT.
Unfortunately, bhyve does not provide a way to probe whether
the open mode is supported, so users will have to make sure
it's supported on their own.
For the reference, without the "open" mode, the guest will have no
outside network connectivity. To make this mode useful,
it is possible to configure forwarding from the host to the guest,
but it is not covered by this patch.
Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
The newly introduced virFileCheckParents is generic enough to be used
for checking whether a specific path or any of its parents is included
in the overrides array.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Switch virFileIsSharedFSOverride to use virFileCheckParents to avoid a
race which could result in virFileCanonicalizePath to be called on a
path that does not exist anymore.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
virFileIsSharedFSType could end up calling statfs on a path that no
longer exists and return an error. If this happens for a path on a
shared filesystem, the caller may incorrectly consider the path as
non-shared.
Specifically, when starting a domain with TPM enabled and deciding
whether its vTPM state is stored on a shared storage, the race could
cause qemuTPMEmulatorBuildCommand to consider the state to be
non-shared. This means swtpm would be started without --migration even
when the state is actually stored on a shared storage and any attempt to
migrate such domain would fail with
Operation not supported: the running swtpm does not support
migration with shared storage
In fact, any caller of virFileGetExistingParent contained an inherent
TOCTOU race condition as the existing parent of a given path return by
virFileGetExistingParent may no longer exist at the time the caller
wants to check it.
This patch introduces a new virFileCheckParents API which is almost
identical to virFileGetExistingParent, but uses a supplied callback to
check each path. This new API is used in virFileIsSharedFSType to avoid
the race. The old function will later be completely removed once all
callers are switched to the new one.
Fixes: 05526b5090
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Technically virFileIsSharedFSOverride is available on any OS, but we
need a mocked realpath() to test it. Because the virfilemock library
also mocks statfs() which is only available on Linux, we don't even try
to load the library anywhere else. Thus we need to skip testing
virFileIsSharedFSOverride on non-Linux too.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Similarly to previous commit, 'flags' is really opaque. The function
lives in migration code and similar functions there expect migration
flags. Here we get virDomainSaveRestoreFlags. Here at least the dump
code handles it properly and passes VIR_DOMAIN_SAVE_BYPASS_CACHE rather
than VIR_DUMP_BYPASS_CACHE.
Note: We, in many cases, encourage use of 'flags' instead of a bunch of
boolean parameters. Since C doesn't do proper type checks on enums and
in fact with 'flags' we pass a binary or of some flags rather than pure
options from the enum there isn't really an elegant solution that would
be enforced by the compiler and easy on eyes. With a bunch of booleans
at least anyone reading the code will need to look up the function
definition to see the header rather than assume that passing in 'flags'
is fine without properly checking *which* flags are accepted by the
function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The meaning of 'flags' is context dependant. 'qemuMigrationSrcToFile'
expects 'virDomainSaveRestoreFlags' rather than migration flags which is
not expected based on the location of the function.
Why this is wrong is clearly visible in 'doCoreDump' which passes in
'dump_flags' which are actually 'virDomainCoreDumpFlags' and the values
are different:
VIR_DUMP_BYPASS_CACHE = (1 << 2)
VIR_DOMAIN_SAVE_BYPASS_CACHE = 1 << 0
Since it checks only for VIR_DOMAIN_SAVE_BYPASS_CACHE pass it in as a
boolean instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The 'VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT' typed parameter for
'virDomainSaveParams' is implemented as a string but really encodes an
enumeration of supported types. We can't change the format any more but
can export the corresponding types as constants.
Additionally this also mentions the missing 'sparse' format.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Extract the definition of the enum into a separate header file and
convert the config struct to use the proper types.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Convert all code refering to the save image type to use the proper enum
value.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If the snapshot has a disk using 'manual' snapshot mode we keep the VM
paused until the user resumes it (presumably after they've done steps to
take the disk snapshot).
Since quiescing is done via the guest agent this means it will not be
possible while the VM is paused.
Rather than trying to implement complex recovery from this state prevent
the use of VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE for the snapshot. The user
still can call virDomainFSFreeze/virDomainFSThaw manually.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
When creating a snapshot with 'VIR_DOMAIN_SNAPSHOT_CREATE_LIVE' the VM
is paused only after dumping the memory state.
This means that also the steps to do a 'manual' disk snapshot
(deactivation of the block nodes in qemu) must happen only once the VM
is paused.
Move the manual snapshot setup code after the memory snapshot code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
A temporary 'qemuFDPass' is used when cleaning up after a migration to a
file but it's not freed after use. Declare it as autoptr.
Fixes: c2518f7bc7
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
A memory device can have <nodemask/> which specifies which host
NUMA nodes the memory should be allocated from (currently
supported for dimm, virtio-mem and sgx-epc models).
But when generating corresponding command line for the device,
the NUMA policy is taken from the guest NUMA node that
corresponds to the memory device (as defined by target/node) or
overall domain NUMA policy (as defined by <numatune/>).
This may lead to memory being allocated from unexpected NUMA
node. For instance, if the memory device has
<nodemask>0</nodemask> and domain has <numatune> <memory
mode='preferred' nodeset='1'/> </numatune> then the cmd line for
the memory device also has just "policy":"preferred".
Treat <nodemask/> as mode='strict'.
But I agree that this is kind of nonsense configuration. Why
would somebody want to prefer one NUMA node but then configure
memory device to allocate NUMA from the other?
Resolves: https://issues.redhat.com/browse/RHEL-114415
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The current logic in 'qemuTPMEmulatorBuildCommand' skips all setup if
the *location* of the data is on what we'd consider shared storage.
This means that if the location is not actually shared (e.g. it's shared
betweeh some other hosts than the two doing the migration) and the path
wasn't ever used (e.g. by migrating out) from the host where we're
migrating into the complete setup of the location would be skipped even
when it doesn't exist.
Fix the logic by skipping only some of the setup steps so that
'qemuTPMEmulatorCreateStorage' can still create the storage if it
doesn't exist.
The rest of the code then needs to take the 'created' flag returned from
'qemuTPMEmulatorCreateStorage' into account.
Fixes: 68103e9daf
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Use presence of non-negative pci_bus to identify multi-smmuv3
IOMMU model, instead of the niommus attribute. This allows for
specifying a single arm-smmuv3 on the qemu command line,
instead of both the virt-machine smmuv3 and arm-smmuv3
being specified at the same time.
Signed-off-by: Nathan Chen <nathanc@nvidia.com>
Fixes: e70c4d54d3 conf: Support multiple device-pluggable smmuv3 IOMMUs
Reviewed-by: Ján Tomko <jtomko@redhat.com>
There's new commit in glibc [1] which makes memchr(), strchr(),
strrchr(), strpbrk() and strstr() reflect type of the input
string. If it's a constant string, then the return type of these
functions is also 'const char *'. But this change tickles
-Wincompatible-pointer-types-discards-qualifiers warning.
And indeed, there are some places where we use a 'char *' typed
variable to store the retval, or even misuse the fact 'char *' is
returned and modify const string.
To fix this, a couple of different approaches is used:
a) switch variable type to 'const char *',
b) switch argument to 'char *' (in a few places we have
strdup()-ed) the const string already,
c) strdup() the string and use b).
1: https://sourceware.org/git/?p=glibc.git;a=commit;h=cd748a63ab1a7ae846175c532a3daab341c62690
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The iptablesPrivateChainCreate() function is given an array of
const strings. This constitutes a promise to the caller that the
data is not modified. But inside the data is modified anyway (to
cut out some parts of the data). Well, with a help from
g_strdup() the promise can be kept.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The body of iptablesPrivateChainCreate() uses STRPREFIX() to
match strings starting with certain prefix. Then it uses pointer
arithmetic to skip the prefix. Well, that's exactly what
STRSKIP() is meant to do. Switch the body to use the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The iptablesPrivateChainCreate() function gets a NULL terminated
array of strings (@lines argument), each item representing one
line of iptables output. Currently, the variable used to iterate
over the array is named 'tmp' which is not very descriptive.
Rename it to 'line'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The aim of virSkipSpacesBackwards() is find the first space
trailing character in given string, For instance, if the input is
"Something whitespacey ", then the output should be pointing to
the very first space after "y".
Problem here is that the input string is constant, but the
returned pointer is non-constant. This is confusing, a caller
shouldn't be able to modify the string, since the input was a
constant string.
Therefore, make the function return a const pointer too.
Under the hood the function used virTrimSpaces() which under some
circumstances could modify the input string. A trick was used to
hide this fact away, but to be double sure rewrite the function's
body.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When the network driver starts up it may inject some firewall
rules (e.g. for a network with NAT). So far, this scenario wasn't
covered in our test suite. The reason for adding this test is
twofold: the first, check we add correct rules, the second is to
cover iptablesPrivateChainCreate() as its implementation is soon
to be changed.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The signature and implementation of virSkipSpacesBackwards() is
soon about to change. Introduce a test case to make sure its
behaviour stays the same.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Throughout years, virCommandSetDryRun() has gained more
functionality and arguments. But:
1) not all arguments are covered in documentation,
2) the example wouldn't even compile.
Expand the documentation to reflect current behaviour.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Tested-by: Jaroslav Suchanek <jsuchane@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
the vmx-* features are currently decoded from the 32-lower bits
(EAX) of the MSR 64-bit values. They should be decoded instead
from the 32-higher bits (EDX).
Signed-off-by: Hector Cao <hector.cao@canonical.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The src/cpu_map/x86_features.xml file contains the definition
of all x86 CPU features, these definitions specify how we can
decode the feature support fom the CPUID or MSR values.
The helper script sync_qemu_features_i386.py builds the
x86_features.xml file from QEMU source code to be in sync
with supported features in QEMU. This helper script parses
QEMU target/i386/cpu.c file looking for CPU feature definitions
and convert them into x86_features.xml contents.
This is the resulting definition for the vmx-intr-exit feature
encoded in the MSR 0x48d.
<!-- msr 0x0000048d -->
<feature name='vmx-intr-exit'>
<msr index='0x0000048d' edx='0x00000000' eax='0x00000001'/>
</feature>
EAX holds the 32 lower bits of the MSRE 64-bits value and should
not be used to detect the VMX-* features. Indeed, VMX-* bit
position should be parsed from QEMU source code in the 32 higher
bits of the corresponding MSR value.
This commit fixes this issue by using the 32 higher bits (EDX)
to represent VMX-* features.
Signed-off-by: Hector Cao <hector.cao@canonical.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Daniel P. Berrangé
Peter Krempa
Roman Bogorodskiy
Martin Kletzander
Andrea Bolognani
Michal Privoznik
Shalini Chellathurai Saroja
Jiri Denemark
Jaak Ristioja
Han Han via Devel
Ján Tomko
Hector Cao
Fco. Javier F. Serrador