Peter Krempa [Wed, 21 Apr 2021 06:49:51 +0000 (08:49 +0200)]
util: xml: Fix confusing semantics of VIR_XML_PROP_OPTIONAL flag
The new enum helpers use a set of flags to modify their behaviour, but
the declared set of flags is semantically confusing:
typedef enum {
VIR_XML_PROP_OPTIONAL = 0, /* Attribute may be absent */
VIR_XML_PROP_REQUIRED = 1 << 0, /* Attribute may not be absent */
Since VIR_XML_PROP_OPTIONAL is declared as 0 any other flag shadows it
and makes it impossible to detect. The functions are not able to detect
a semantic nonsense of VIR_XML_PROP_OPTIONAL | VIR_XML_PROP_REQUIRED and
it's a perfectly valid statement for the compilers.
In general having two flags to do the same boolean don't make sense and
the implementation doesn't fix any shortcomings either.
To prevent mistakes, rename VIR_XML_PROP_OPTIONAL to VIR_XML_PROP_NONE,
so that there's always an enum value used with the calls but it doesn't
imply that the flag makes the property optional when the actual value is
0.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Tim Wiederhake [Tue, 20 Apr 2021 11:27:52 +0000 (13:27 +0200)]
testUpdateQEMUCaps: Fix memory leak
testUpdateQEMUCaps is called multiple times. Use virQEMUCapsUpdateHostCPUModel
instead of virQEMUCapsInitHostCPUModel to not overwrite (and leak) the
pointers in qemuCaps->kvm.hostCPU and qemuCaps->tcg.hostCPU.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Fixes: 4eb7c621985dad4de911ec394ac628bd1a5b29ab Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
We can figure out the appropriate value for 'create' from the command
type, so push that into the test function rather than specifying it in
the test macro.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
Jonathon Jongsma [Wed, 31 Mar 2021 22:03:08 +0000 (17:03 -0500)]
nodedev: Remove GetMdevctl*Command() wrappers
These per-command generator functions were only exposed in the header to
allow the commandline generation to be tested. Now that we have a
generic mdevctl command generator, we can get rid of the per-command
wrappers and reduce the noise in the header.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Erik Skultety [Wed, 31 Mar 2021 13:24:47 +0000 (15:24 +0200)]
nodedev: driver: Create a generic mdevctl command translator
Currently there are dedicated wrappers to construct mdevctl command.
These are mostly fine except for the one that translates both "start"
and "define" commands, only because mdevctl takes the same set of
arguments. Instead, keep the wrappers, but let them call a single
global translator that handles all the mdevctl command differences and
commonalities.
Signed-off-by: Erik Skultety <eskultet@redhat.com> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
This is not a 1:1 mapping to mdevctl commands because mdevctl doesn't
support a separate 'create' command. mdevctl uses 'start' for both
starting a pre-defined device as well as for creating and starting a new
transient device. The libvirt code will be more readable if we treat
these as separate commands. When we need to actually execute mdevctl,
the 'create' command will be translated into the appropriate 'mdevctl
start' command.
Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
Erik Skultety [Wed, 31 Mar 2021 13:32:44 +0000 (15:32 +0200)]
nodedev: driver: Swap virMdevctlStart and virMdevctlCreate
"start" in libvirt means - "take this object and create an
instance out of it"
"create" in libvirt most of the time means - "take and XML description,
make an object out of it and use it to create an instance"
This gets confusing with mdevctl which uses "start" for both. So, this
patch proposes to use virMdevctlStart in cases where from libvirt's POV
we're starting a defined device (unlike mdevctl). Similarly, use
virMdevctlCreate in scenarios where XML description is passed to
libvirt and a transient device is supposed to be created.
Signed-off-by: Erik Skultety <eskultet@redhat.com> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
tests: nodedev: switch all test macros to accept a filename
Rather than specifying a UUID string to some test macros, just pass a
filename to an xml definition. This helps work toward unifying the test
macros and making it more maintainable.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
Jonathon Jongsma [Tue, 30 Mar 2021 15:12:07 +0000 (10:12 -0500)]
nodedev: avoid use of VIR_ERR_NO_* errors internally
These errors are demoted to debug statements[1] since they're only
intended to be used as return values for public APIs. This makes it
difficult to debug the problem when something goes wrong since no error
message is logged. Switch instead to VIR_ERR_INTERNAL_ERROR so that the
error is logged as expected.
[1] See the implementation of daemonErrorLogFilter() for details:
https://gitlab.com/libvirt/libvirt/-/blob/e2f82a3704f680fbb37a733476d870c19232c23e/src/remote/remote_daemon.c#L89
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
Peter Krempa [Fri, 16 Apr 2021 07:10:55 +0000 (09:10 +0200)]
conf: domain: Refactor virDomainDiskDefParseXML
Use the new virXMLProp helpers and XPath queries to get rid of the old
style of iteration through element children.
Note that in case of def->blockio.logical_block_size,
def->blockio.physical_block_size and def->rotation_rate the wraparound
behaviour of 'virStrToLong_ui' was _not_ forward ported to the new code
as it makes no sense with the attributes.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Peter Krempa [Fri, 16 Apr 2021 08:46:00 +0000 (10:46 +0200)]
conf: domain: Convert virDomainDiskDef's 'snapshot' to unsigned int
Unfortunately virDomainSnapshotLocation is declared in snapshot_conf.h
which includes domain_conf.h. To avoid a circular dependency use
'unsigned int' for now.
Use XML parser can use virXMLPropEnum.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Peter Krempa [Thu, 15 Apr 2021 15:15:26 +0000 (17:15 +0200)]
conf: domain: Move default setting from virDomainDiskDefParseXML to virDomainDiskDefPostParse
Move the setting of read-only state, the default disk bus and setting of
'snapshot' state for read-only disks to the post parse callback to clean
up the disk parser.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Peter Krempa [Wed, 14 Apr 2021 14:48:53 +0000 (16:48 +0200)]
conf: domain: Introduce an internal variant of virDomainDiskDefNew
The <disk> XML element parser is going to be modified so that the
virStorageSource bits are pre-parsed. Add virDomainDiskDefNewSource,
which uses an existing 'src' pointer.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Peter Krempa [Wed, 14 Apr 2021 14:43:22 +0000 (16:43 +0200)]
qemu: driver: Use virDomainDiskDefParseSource in qemuDomainBlockCopy
qemuDomainBlockCopy needs just the source portion of the disk but uses
the disk parser for it. Since we have a specific function now, refactor
the code to avoid having to deal with the unused virDomainDiskDef.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Michal Privoznik [Sat, 17 Apr 2021 07:59:51 +0000 (09:59 +0200)]
lxc: Format --handshakefd for controller cmd fully
The command line argument is called --hanshakefd (check out
lxc_controller.c:main()). But the command line builder puts only
--handshake. This works, because there is no other argument
sharing the prefix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
I've encountered the following bug, but only on Gentoo with
systemd and CGroupsV2. I've started an LXC container successfully
but destroying it reported the following error:
error: Failed to destroy domain 'amd64'
error: internal error: failed to get cgroup backend for 'pathOfController'
Debugging showed, that CGroup hierarchy is full of surprises:
Anyway, those nested directories should not be a problem, because
virCgroupKillRecursiveInternal() removes them recursively, right?
Sort of. The function really does remove nested directories, but
it assumes that every directory has the same controller as the
rest. Just take a look at virCgroupV2KillRecursive() - it gets
'Any' controller (the first one it found in ".scope") and then
passes it to virCgroupKillRecursiveInternal().
This assumption is not true though. The controllers found in
".scope" are the following:
cpuset cpu io memory pids
while "libvirt" has fewer:
cpuset cpu io memory
Up until now it's not problem, because of how we order
controllers internally - "cpu" is the first and thus picking
"Any" controller returns just that. But the rest of directories
has no controllers, their "cgroup.controllers" is just empty.
What fixes the bug is dropping @controller argument from
virCgroupKillRecursiveInternal() and letting each iteration work
pick its own controller.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Michal Privoznik [Fri, 16 Apr 2021 14:14:33 +0000 (16:14 +0200)]
vircgroupbackend: Extend error messages in VIR_CGROUP_BACKEND_CALL()
The VIR_CGROUP_BACKEND_CALL() macro gets a backend for controller
and calls corresponding callback in it. If either is NULL then an
error message is printed out. However, the error message contains
only the intended callback func and not controller or backend
found.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Peter Krempa [Mon, 19 Apr 2021 06:11:55 +0000 (08:11 +0200)]
cmdDomBlkError: Fix crash when initial call to virDomainGetDiskErrors fails
virDomainGetDiskErrors uses the weird semantics where we make the
caller query for the number of elements and then pass pre-allocated
structure.
The cleanup section errorneously used the 'count' variable to free the
allocated elements for the API but 'count' can be '-1' in cases when the
API returns failure, thus attempting to free beyond the end of the
array.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/155 Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Peter Krempa [Thu, 15 Apr 2021 11:26:37 +0000 (13:26 +0200)]
virsh: snapshot: Don't validate schema of XML generated by 'virsh snapshot-create-as'
Commit 95f8e3237e5486f487324c6 which introduced XML schema validation
for snapshot XMLs always asserted the validation for the XML generated
by 'virsh snapshot-create-as' on the basis that it's libvirt-generated,
thus valid.
This unfortunately isn't true as users can influence certain bits of the
XML such as the disk image path which must be a full path. Thus if a
user tries to invoke virsh as:
$ virsh snapshot-create-as upstream --diskspec vda,file=relative.qcow2
error: XML document failed to validate against schema: Unable to validate doc against /path/to/domainsnapshot.rng
Extra element disks in interleave
Element domainsnapshot failed to validate content
They get a rather useless error from the libxml2 RNG validator.
With this fix applied, we get to the XML parser in libvirtd which has a
more reasonable error:
$ virsh snapshot-create-as upstream --diskspec vda,file=relative.qcow2
error: XML error: disk snapshot image path 'relative.qcow2' must be absolute
Instead users can force validation of the XML generated by 'virsh
snapshot-create-as' by passing the '--validate' flag.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Peter Krempa [Wed, 14 Apr 2021 11:03:58 +0000 (13:03 +0200)]
virXMLParseHelper: Rework error reporting
Move the reporting of parsing error on the error path of the parser as
other code paths report their own errors already.
Additionally prefer printing the 'url' as document name if provided
instead of "[inline data]" as that usually gives a better hint at least
which kind of XML is being parsed.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
Currently virMediatedDeviceGetIOMMUGroupDev() looks up the iommu group
number and uses that to construct a path to the iommu group device.
virMediatedDeviceGetIOMMUGroupNum() then uses that device path and takes
the basename to get the group number. That's unnecessary extra string
manipulation for *GroupNum(). Reverse the implementations and make
*GroupDev() call *GroupNum().
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
Tim Wiederhake [Thu, 15 Apr 2021 08:12:13 +0000 (10:12 +0200)]
virlog: Remove stray "todo" in comment
Fixes: 8fe30b2167b5b56461b11dbf02aca83030070caf Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Ján Tomko <jtomko@redhat.com>
Jim Fehlig [Tue, 13 Apr 2021 23:29:19 +0000 (17:29 -0600)]
libxl: Add debug statements
Over several years of debugging reports related to VM shutdown, destruction,
and cleanup, I've found that logging of all events received from libxl and
logging the entry of libxlDomainCleanup has proven useful. Add the these
debug messages upstream to aid in future debugging.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
projects / libvirt.git / log