Peter Krempa [Fri, 29 Mar 2019 08:05:45 +0000 (09:05 +0100)]
qemu: caps: Separate capabilities based on qemu version
virQEMUCapsInitQMPMonitor is massive now since it collects calls to the
various probing functions and also version based capabilities. Split
out the version based caps into a separate function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
Ján Tomko [Fri, 29 Mar 2019 13:36:57 +0000 (14:36 +0100)]
virsh-completer: use VIR_AUTOSTRINGLIST for tmp
We've been open-coding virStringListFreeCount for cleaning up
the completion list we're building. This had the advantage of
zeoring the pointer afterwards, which is no longer needed
now that we compile the list in 'tmp' instead of 'ret'.
Since all our lists are NULL-terminated anyway, switch to using
virStringListFree via the VIR_AUTOSTRINGLIST macro.
Fixes nearly impossible NULL dereferences in
virshNWFilterBindingNameCompleter
virshNWFilterNameCompleter
virshNodeDeviceNameCompleter
virshNetworkNameCompleter
virshInterfaceNameCompleter
virshStoragePoolNameCompleter
virshDomainNameCompleter
which jumped on the error label after a failed allocation
and a possible one in
virshStorageVolNameCompleter
which jumped there when we fail to fetch the list of volumes.
Ján Tomko [Thu, 28 Mar 2019 17:26:44 +0000 (18:26 +0100)]
virsh-completer: add a cleanup label everywhere
Unify the cleanup paths for error and success.
Now that 'ret' is only set (from tmp) on the success path,
it is safe to jump right before 'return ret' after processing
the error block.
Andrea Bolognani [Tue, 12 Mar 2019 16:21:37 +0000 (17:21 +0100)]
tests: Don't use canonical paths in virstoragetest
The layout of my home directory is somewhat peculiar: I store
all git repositories in ~/src/upstream, but since I spend
almost all of my time hacking on libvirt, I also have a
convenience symlink ~/src/libvirt -> ~/src/upstream/libvirt
that I use to access that specific git repository.
The above setup has served me well for years; however, ever
since commit ca1471622dd9 dropped our own custom definitions
for abs_{,top_}{src,build}dir and started using the ones
provided by autotools, virstoragetest has started reliably
failing with errors such as
Using abolute paths instead of canonical ones in the tests makes
the problem go away.
Note that all tests that are specifically designed to test path
canonicalization via TEST_PATH_CANONICALIZE() were passing even
before this patch and are not touched by it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Acked-by: Michal Privoznik <mprivozn@redhat.com>
This file contains the old school ChangeLog, which was manually
updated for every set of changes before the switch to git.
When libvirt was imported into git, however, *all* history was
preserved, including the changes documented in this file, and
can still be inspected using 'git log' just like more recent
changes: the format might be slightly different, but that's not
quite reason enough to treat this file any differently than the
git-generated ChangeLog we just dropped.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Our ChangeLog is generated by basically redirecting the output
of 'git log' into it so, as can be expected, it has only gotten
bigger as development has progressed. As of today, its size has
reached pretty much comical levels:
$ du -sk ChangeLog
11328 ChangeLog
All of that for information *literally nobody* cares about: end
users and distro maintainers have proper release notes lovingly
compiled for them, while developers peruse the history either by
calling 'git log' directly or through their favorite $EDITOR's
git integration.
Replacing the generated ChangeLog with a short message pointing
interested parties to the git repository does not only reduce
the size of the unpacked sources from 259904 KiB to 248576 KiB
(~4% saving): from a quick test on my laptop, doing so reduces
the size of the *compressed* release archive from 15140 KiB to
12364 KiB (~18% saving) and also takes the time needed to run
'make distcheck' down from 4:44 to 4:21 (~8% saving).
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
The VM Manager app is no longer present on the Play store and while
Google shows a couple of hits they look like the typical untrustworthy
3rd party download redistributors rather than an official site.
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This reverts commit 047cfb05ee949325e77726531fd381820be8dc62
Using numeric comparison on strings means we reject every update
that does include the group name, even if it's unchanged.
Signed-off-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Laine Stump <laine@laine.org>
Eric Blake [Thu, 28 Mar 2019 02:15:43 +0000 (21:15 -0500)]
virsh: Don't infloop on snapshot/storage_vol failure
Most of our completers used the pattern:
if ((nITEM = virITEMListAll()) < 0)
return NULL;
but the virDomainSnapshot and virStorageVolume completers were instead
using goto error. If the ListAll fails with -1, the cleanup label was
running a loop of 'size_t i < int nITEM', which is an extreme waste of
CPU cycles. Broken since their introduction in v4.1.
Fixes: f81f8b62 Fixes: 4cb4b649 Reported-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>
Eric Blake [Thu, 28 Mar 2019 14:00:59 +0000 (09:00 -0500)]
snapshot: Improve logic of virDomainMomentMoveChildren
Even though Coverity can prove that 'last' is always set if the prior
loop executed, gcc 8.0.1 cannot:
CC conf/libvirt_conf_la-virdomainmomentobjlist.lo
../../src/conf/virdomainmomentobjlist.c: In function 'virDomainMomentMoveChildren':
../../src/conf/virdomainmomentobjlist.c:178:19: error: 'last' may be used uninitialized in this function [-Werror=maybe-uninitialized]
last->sibling = to->first_child;
~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
Rewrite the loop to a form that should be easier for static analysis
to work with.
Fixes: ced0898f86bf Reported-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Laine Stump [Wed, 27 Mar 2019 18:58:45 +0000 (14:58 -0400)]
util: suppress unimportant ovs-vsctl errors when getting interface stats
commit edaf13565 modified the stats retrieval for OVS interfaces to
not fail when one of the fields was unrecognized by the ovs-vsctl
command, but ovs-vsctl was still returning an error, and libvirt was
cluttering the logs with these inconsequential error messages.
This patch modifies the GET_STAT macro to add "--if-exists" to the
ovs-vsctl command, which causes it to return an empty string (and exit
with success) if the requested statistic isn't in its database, thus
eliminating the ugly error messages from the log.
Peter Krempa [Thu, 28 Mar 2019 12:12:32 +0000 (13:12 +0100)]
qemu: Always use 'alias' in warning message when removing USB address
Avoid the extra parameter passing in the disk 'dst' parameter to be
reported instead of the device alias. Using 'dst' instead of alias does
not add much value.
Michal Privoznik [Wed, 27 Mar 2019 16:30:44 +0000 (17:30 +0100)]
qemuxml2argvtest: Drop dependency between testInfoArgName and virQEMUCapsFlags enums
Introduced in fdf6c89ee7b, this dependency looks weird. It was
needed because of the way that while() loop was written - it
fetches next argument in every iteration. Therefore, our only
option was for ARG_END to have the same value as QEMU_CAPS_LAST.
This also meant that QEMU_CAPS_* could have been only at the end
of the __VA_ARGS__.
This commit reworks the while() loop and removes the dependency.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Michal Privoznik [Wed, 27 Mar 2019 16:19:37 +0000 (17:19 +0100)]
qemu_capabilities; Drop virQEMUCapsSetVAList
There is one specific caller (testInfoSetArgs() in
qemuxml2argvtest.c) which expect the va_list argument to change
after returning from the virQEMUCapsSetVAList() function.
However, since we are passing plain va_list this is not
guaranteed. The man page of stdarg(3) says:
If ap is passed to a function that uses va_arg(ap,type), then
the value of ap is undefined after the return of that function.
(ap is a variable of type va_list)
I've seen this in action in fact: on i686 the qemuxml2argvtest
fails on the second test case because testInfoSetArgs() sees
ARG_QEMU_CAPS and calls virQEMUCapsSetVAList to process the
capabilities (in this case there's just one
QEMU_CAPS_SECCOMP_BLACKLIST). But since the changes are not
reflected in the caller, in the next iteration testInfoSetArgs()
sees the QEMU capability and not ARG_END.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The mock fopen() function will abort if "/proc/mounts" is
requested with "r" permissions and VIR_CGROUP_MOCK_FILENAME
env var is not set.
Unfortunately this is triggering by the libselinux library
constructor when it tries to read /proc/mounts to find out
if selinuxfs is mounted in an unusual place.
This, however, only affects libselinux in Debian as that
opens with "r", while in Fedora / RHEL it opens "re" and
thus luckily never triggered the abort(), instead getting
an EACCESS.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
It turns out gcc -O2 is not happy with it, complaining:
/home/pipo/libvirt/src/qemu/qemu_driver.c: In function 'qemuDomainSnapshotCreateXML':
/home/pipo/libvirt/src/qemu/qemu_driver.c:15389:26: error: potential null pointer dereference [-Werror=null-dereference]
bool memory = snapdef->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
~~~~~~~^~~~~~~~
/home/pipo/libvirt/src/qemu/qemu_driver.c:15389:26: error: potential null pointer dereference [-Werror=null-dereference]
In file included from /home/pipo/libvirt/src/util/virbuffer.h:27,
from /home/pipo/libvirt/src/conf/capabilities.h:27,
from /home/pipo/libvirt/src/conf/domain_conf.h:32,
from /home/pipo/libvirt/src/qemu/qemu_agent.h:26,
from /home/pipo/libvirt/src/qemu/qemu_driver.c:40:
/home/pipo/libvirt/src/util/viralloc.h:125:34: error: potential null pointer dereference [-Werror=null-dereference]
# define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count), true, \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/pipo/libvirt/src/qemu/qemu_driver.c:15103:9: note: in expansion of macro 'VIR_ALLOC_N'
if (VIR_ALLOC_N(ret, snapdef->ndisks) < 0)
^~~~~~~~~~~
/home/pipo/libvirt/src/qemu/qemu_driver.c:15798:45: error: null pointer dereference [-Werror=null-dereference]
virDomainSnapshotObjGetDef(snap)->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
As the patch simplified one or two callers at the risk of making
many other callers now candidates to trigger aggressive compiler
warnings, it isn't worth it.
Eric Blake [Wed, 27 Mar 2019 07:12:37 +0000 (02:12 -0500)]
snapshot: Refactor qemu to utilize virDomainMoment more
Use the common base class virDomainMoment for iterator callbacks
related to snapshots from the qemu code, so that when checkpoint
operations are introduced, they can share the same callbacks.
Simplify the code for qemuDomainSnapshotCurrent by better utilizing
virDomainMoment helpers.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Eric Blake [Wed, 27 Mar 2019 08:17:46 +0000 (03:17 -0500)]
snapshot: Drop pointless function virDomainMomentIsCurrentName
The qemu driver already had a full-blown virDomainMomentObjPtr to
check against, and the test driver ought to have one since we get
better error checking that the user passed in a valid object. Removes
the need for a helper function added in commit commit 4819f54b.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Andrea Bolognani [Mon, 25 Mar 2019 09:51:05 +0000 (10:51 +0100)]
spec: Move ldconfig calls from -client to -libs
ldconfig needs to be called after installing or uninstalling
shared libraries.
For a very long time, libvirt didn't have a separate package
containing just the shared libraries, and so it shipped them
in the same one as the clients.
Since commit 70b4f0e719cd, however, shared libraries have been
moved from -client to their own -libs package; unfortunately,
the corresponding ldconfig calls were not moved at the same
time, which is what this commit takes care of.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Jiri Denemark [Wed, 6 Feb 2019 10:36:36 +0000 (11:36 +0100)]
Public API for parallel migration
This patch adds a new VIR_MIGRATE_PARALLEL flag for migration APIs which
will ask the hypervisor to use multiple parallel connections for
migrating a domain. The number of parallel connections can be set using
VIR_MIGRATE_PARAM_PARALLEL_CONNECTIONS typed parameter.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Eric Blake [Thu, 3 Jan 2019 01:55:25 +0000 (19:55 -0600)]
backup: Introduce virDomainCheckpointPtr
Prepare for introducing a bunch of new public APIs related to
backup checkpoints by first introducing a new internal type
and errors associated with that type. Checkpoints are modeled
heavily after virDomainSnapshotPtr (both represent a point in
time of the guest), although a snapshot exists with the intent
of rolling back to that state, while a checkpoint exists to
make it possible to create an incremental backup at a later
time.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Eric Blake [Tue, 26 Mar 2019 05:20:04 +0000 (00:20 -0500)]
snapshot: Various doc tweaks
Since I was copying this text to form checkpoint XML and API
documentation, I might as well make improvements along the way. Most
of these changes are based on reviews of the checkpoint docs.
Among other things: grammar tweaks, point to a single source of
documentation rather than repeating verbosity, reword things for
easier legibility.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This reverts commit 86c0ed6f70268dfa7c3bba95a0ba96fcfe2ab039, and
subsequent refactorings of the function into new files. There are no
callers of this function - I had originally proposed it for
implementing a new bulk snapshot API, but that proved to be too
invasive given RPC limits. I also tried using it for streamlining how
the qemu driver stores snapshot state across libvirtd restarts
internally, but in the end, the risks of a new internal format
outweighed the benefits of one file per snapshot.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
This reverts commit 1b57269cbcfcfe998a065c0c9f0f8db408710d87, and
subsequent refactorings of the function into new files. There are no
callers of this function - I had originally proposed it for
implementing a new bulk snapshot API, but that proved to be too
invasive given RPC limits. I also tried using it for streamlining how
the qemu driver stores snapshot state across libvirtd restarts
internally, but in the end, the risks of a new internal format
outweighed the benefits of one file per snapshot.
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Andrea Bolognani [Tue, 26 Mar 2019 10:01:32 +0000 (11:01 +0100)]
m4: Run QEMU under a distro-specific user when possible
Our current defaults are root:wheel on FreeBSD and macOS, root:root
everywhere else.
Looking at what downstream distributions actually do, we can see that
these defaults are overriden the vast majority of the time, with a
number of variations showing up in the wild:
* qemu:qemu -> Used by CentOS, Fedora, Gentoo, OpenSUSE, RHEL
and... As it turns out, our very own spec file :)
* libvirt-qemu:libvirt-qemu -> Used by Debian.
* libvirt-qemu:kvm -> Used by Ubuntu.
* nobody:nobody -> Used by Arch Linux.
Based on this information, we can do a better job at integrating with
downstream packages: if the distro-specific user and group already
exist on the system then we use them, and if not (or we're building
on an unknown OS) we just use root:root as we would have before.
This change makes it less likely that people building from source
will end up running their guests as root, which is a very desiderable
outcome from the security point of view.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Laine Stump [Mon, 25 Mar 2019 14:46:56 +0000 (10:46 -0400)]
qemu_hotplug: don't shutdown net device until the guest has released it
For [some unknown reason, possibly/probably pure chance], Net devices
have been taken offline and their bandwidth tc rules cleared as the
very first operation when detaching the device. This is contrary to
every other type of device, where all hostside teardown is delayed
until we receive the DEVICE_DELETED event back from qemu, indicating
that the guest has finished with the device.
This patch delays these two operations until receipt of
DEVICE_DELETED, which removes an ugly wart from
qemuDomainDetachDeviceLive(), and also seems to be a more correct
sequence of events.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Thu, 21 Mar 2019 16:54:10 +0000 (12:54 -0400)]
qemu_hotplug: delay sending DEVICE_REMOVED event until after *all* teardown
The VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event is sent after qemu has
responded to a device_del command with a DEVICE_DELETED event. Before
queuing the event, *some* of the final teardown of the device's
trappings in libvirt is done, but not *all* of it. As a result, an
application may receive and process the DEVICE_REMOVED event before
libvirt has really finished with it.
Usually this doesn't cause a problem, but it can - in the case of the
bug report referenced below, vdsm is assigning a PCI device to a guest
with managed='no', using livirt's virNodeDeviceDetachFlags() and
virNodeDeviceReAttach() APIs. Immediately after receiving a
DEVICE_REMOVED event from libvirt signalling that the device had been
successfully unplugged, vdsm would cal virNodeDeviceReAttach() to
unbind the device from vfio-pci and rebind it to the host driverm but
because the event was received before libvirt had completely finished
processing the removal, that device was still on the "activeDevs"
list, and so virNodeDeviceReAttach() failed.
Experimentation with additional debug logs proved that libvirt would
always end up dispatching the DEVICE_REMOVED event before it had
removed the device from activeDevs (with a *much* greater difference
with managed='yes', since in that case the re-binding of the device
occurred after queuing the device).
Although the case of hostdev devices is the most extreme (since there
is so much involved in tearing down the device), *all* device types
suffer from the same problem - the DEVICE_REMOVED event is queued very
early in the qemuDomainRemove*Device() function for all of them,
resulting in a possibility of any application receiving the event
before libvirt has really finished with the device.
The solution is to save the device's alias (which is the only piece of
info from the device object that is needed for the event) at the
beginning of processing the device removal, and then queue the event
as a final act before returning. Since all of the
qemuDomainRemove*Device() functions (except
qemuDomainRemoveChrDevice()) are now called exclusively from
qemuDomainRemoveDevice() (which selects which of the subordinates to
call in a switch statement based on the type of device), the shortest
route to a solution is to doing the saving of alias, and later
queueing of the event, in the higher level qemuDomainRemoveDevice(),
and just completely remove the event-related code from all the
subordinate functions.
The single exception to this, as mentioned before, is
qemuDomainRemoveChrDevice(), which is still called from somewhere
other than qemuDomainRemoveDevice() (and has a separate arg used to
trigger different behavior when the chr device has targetType ==
GUESTFWD), so it must keep its original behavior intact, and must be
treated differently by qemuDomainRemoveDevice() (similar to the way
that qemuDomainDetachDeviceLive() treats chr and lease devices
differently from all the others).
Laine Stump [Thu, 21 Mar 2019 01:44:00 +0000 (21:44 -0400)]
qemu_hotplug: consolidate all common detach code in qemuDomainDetachDeviceLive
Now that all the qemuDomainDetachPrep*() functions look nearly
identical at the end, we can put one copy of that identical code in
qemuDomainDetachDeviceLive() at the point after the individual prep
functions have been called, and remove the duplicated code from all
the prep functions. The code to locate the target "detach" device
based on the "match" device remains, as do all device-type-specific
validations.
Unfortunately there are a few things going on at once in this patch,
which makes it a bit more difficult to follow than the others; it was
just impossible to do the changes in stages and still have a
buildable/testable tree at each step.
The other changes of note:
* The individual prep functions no longer need their driver or async
args, so those are removed, as are the local "ret" variables, since
in all cases the functions just directly return -1 or 0.
* Some of the prep functions were checking for a valid alias and/or
for attempts to detach a multifunction PCI device, but not all. In
fact, both checks are valid (or at least harmless) for *all* device
types, so they are removed from the prep functions, and done a
single time in the common function.
(any attempts to *create* an alias when there isn't one has been
removed, since that is doomed to failure anyway; the only way the
device wouldn't have an alias is if 1) the domain was created by
calling virsh qemu-attach to attach an existing qemu process to
libvirt, and 2) the qemu command that started said process used "old
style" arguments for creating devices that didn't have any device
ids. Even if we constructed a device id for one of these devices,
qemu wouldn't recognize it in the device_del command anyway, so we
may as well fail earlier with "device missing alias" rather than
failing later with "couldn't delete device net0".)
* Only one type of device has shutdown code that must not be called
until after *all* validation of the device is done (including
checking for multifunction PCI and valid alias, which is done in the
toplevel common code). For this reason, the Net function has been
split in two, with the 2nd half (qemuDomainDetachShutdownNet())
called from the common function, right before sending the delete
command to qemu.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Mon, 25 Mar 2019 14:23:51 +0000 (10:23 -0400)]
qemu_hotplug: audit *all* auditable device types in qemuDomainRemoveAuditDevice
Although all hotpluggable devices other than lease, controller,
watchdof, and vsock can be audited, and *are* audited when an unplug
is successful, only disk, net, and hostdev were actually being audited
on failure.
This patch corrects that omission.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Wed, 20 Mar 2019 23:44:05 +0000 (19:44 -0400)]
qemu_hotplug: new function qemuDomainRemoveAuditDevice()
This function can be called with a virDomainDevicePtr and whether or
not the removal was successful, and it will call the appropriate
virDomainAudit*() function with the appropriate args for whatever type
of device it's given (or do nothing, if that's appropriate). This
permits generalizing some code that currently has a separate copy for
each type of device.
NB: Although the function initially will be called only with
success=false, that has been made an argument so that in the future
(when the qemuDomainRemove*Device() functions have had their common
functionality consolidated into qemuDomainRemoveDevice()), this new
common code can call qemuDomainRemoveAuditDevice() for all types.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Wed, 20 Mar 2019 18:32:53 +0000 (14:32 -0400)]
qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()
Most of these functions will soon contain only some setup for
detaching the device, not the detach code proper (since that code is
identical for these devices). Their device specific functions are all
being renamed to qemuDomainDetachPrep*(), where * is the
name of that device's data member in the virDomainDeviceDef
object.
Since there will be other code in qemuDomainDetachDeviceLive() after
the calls to qemuDomainDetachPrep*() that could still fail, we no
longer directly set "ret" with the return code from
qemuDomainDetachPrep*() functions, but simply return -1 on
failure, and wait until the end of qemuDomainDetachDeviceLive() to set
ret = 0.
Along with the rename, qemuDomainDetachPrep*() functions are also
given similar arglists, including an arg called "match" that points to
the proto-object of the device we want to delete, and another arg
"detach" that is used to return a pointer to the actual object that
will be (for now *has been*) detached. To make sure these new args
aren't confused with existing local pointers that sometimes had the
same name (detach), the local pointer to the device is now named after
the device type ("controller", "disk", etc). These point to the same
place as (*detach)->data.blah, it's just easier on the eyes to have,
e.g., "disk->dst" rather than "(*detach)->data.disk-dst".
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Mon, 25 Mar 2019 00:59:21 +0000 (20:59 -0400)]
qemu_hotplug: separate Chr|Lease from other devices in DetachDevice switch
The Chr and Lease devices have detach code that is too different from
the other device types to handle with common functionality (which will
soon be added at the end of qemuDomainDetachDeviceLive(). In order to
make this difference obvious, move the cases for those two device
types to the top of the switch statement in
qemuDomainDetachDeviceLive(), have the cases return immediately so the
future common code at the end of the function will be skipped, and
also include some hopefully helpful comments to remind future
maintainers why these two device types are treated differently.
Any attempt to detach an unsupported device type should also skip the
future common code at the end of the function, so the case for
unsupported types is similarly changed from a simple break to a return
-1.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Tue, 19 Mar 2019 21:15:00 +0000 (17:15 -0400)]
qemu_hotplug: rename dev to match in qemuDomainDetachDeviceLive
I'm about to add a second virDomainDeviceDef to this function that
will point to the actual device in the domain object. while this is
just a partially filled-in example of what to look for. Naming it
match will make the code easier to follow.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Sat, 23 Mar 2019 16:29:23 +0000 (12:29 -0400)]
qemu_hotplug: make Detach functions called only from qemu_hotplug.c static
These are no longer called from qemu_driver.c, since the function that
called them (qemuDomainDetachDeviceLive()) has been moved to
qemu_hotplug.c, and they are no longer called from testqemuhotplug.c
because it now just called qemuDomainDetachDeviceLive() instead of all
the subordinate functions.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Tue, 19 Mar 2019 18:47:40 +0000 (14:47 -0400)]
test: replace calls to individual detach functions with one call to main detach
The individual qemuDomainDetach*Device() functions will soon be "less
functional", since some of the code that is duplicated in 10 of the 12
detach functions is going to be moved into the common
qemuDomainDetachDeviceLive(), which calls them all.
qemuhotplugtest.c is the only place any of these individual functions
is called other than qemuDomainDetachDeviceLive() itself. Fortunately,
qemuDomainDetachDeviceLive() provides exactly the functionality needed
by the test driver (except that it supports detach of more device
types than the test driver has tests for).
This patch replaces the calls to
qemuDomainDetach(Chr|Shmen|Watchdog|Disk)Device with a single call to
the higher level function, allowing us to shift functionality between
the lower level functions without breaking the tests.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Tue, 19 Mar 2019 18:06:05 +0000 (14:06 -0400)]
qemu_hotplug: pull qemuDomainUpdateDeviceList out of qemuDomainDetachDeviceLive
qemuDomainDetachDeviceLive() is called from two places in
qemu_driver.c, and qemuDomainUpdateDeviceList() is called from the
end of qemuDomainDetachDeviceLive(), which is now in qemu_hotplug.c
This patch replaces the single call to qemuDomainUpdateDeviceList()
with two calls to it immediately after return from
qemuDomainDetachDeviceLive(). This is only done if the return from
that function is exactly 0, in order to exactly preserve previous
behavior.
Removing that one call from qemuDomainDetachDeviceList() will permit
us to call it from the test driver hotplug test, replacing the
separate calls to qemuDomainDetachDeviceDiskLive(),
qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and
qemuDomainDetachWatchdog(). We want to do this so that part of the
common functionality of those three functions (and the rest of the
device-specific Detach functions) can be pulled up into
qemuDomainDetachDeviceLive() without breaking the test. (This is done
in the next patch).
NB: Almost certainly this is "not the best place" to call
qemuDomainUpdateDeviceList() (actually, it is provably the *wrong*
place), since it's purpose is to retrieve an "up to date" list of
aliases for all devices from qemu, and if the guest OS hasn't yet
processed the detach request, the now-being-removed device may still
be on that list. It would arguably be better to instead call
qemuDomainUpdateDevicesList() later during the response to the
DEVICE_DELETED event for the device. But removing the call from the
current point in the detach could have some unforeseen ill effect due
to changed timing, so the change to move it into
qemuDomainRemove*Device() will be done in a separate patch (in order
to make it easily revertible in case it causes a regression).
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Tue, 19 Mar 2019 21:37:55 +0000 (17:37 -0400)]
qemu_hotplug: remove extra function in middle of DetachController call chain
qemuDomainDetachDeviceControllerLive() just checks if the controller
type is SCSI, and then either returns failure, or calls
qemuDomainDetachControllerDevice().
Instead, lets just check for type != SCSI at the top of the latter
function, and call it directly.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Tue, 19 Mar 2019 17:55:13 +0000 (13:55 -0400)]
qemu_hotplug: move qemuDomainDetachDeviceLive() to qemu_hotplug.c
This function is going to take on some of the functionality of its
subordinate functions, which all live in qemu_hotplug.c.
qemuDomainDetachDeviceControllerLive() is only called from
qemuDomainDetachDeviceLive() (and will soon be merged into
qemuDomainDetachControllerDevice(), which is in qemu_hotplug.c), so
it is also moved.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Eric Blake [Fri, 22 Mar 2019 04:17:01 +0000 (23:17 -0500)]
snapshot: Make virDomainMomentObjListGetNames more generic
Rather than hard-coding the snapshot filter bit values into the
generic code, add another layer of indirection: callers must map which
of their public filter bits correspond to supported moment bits, then
pass two separate flags (the ones translated for moment code to
operate on, and the remaining ones for the filter callback to operate
on).
Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com>
Laine Stump [Tue, 19 Mar 2019 17:42:55 +0000 (13:42 -0400)]
qemu_hotplug: move (Attach|Detach)Lease functions with others of same type
The Attach and Detach Lease functions were together in the middle of
the Detach functions. Put them at the end of their respective
sections, since they behave differently from the other attach/detach
functions (DetachLease doesn't use qemuDomainDeleteDevice(), and is
always synchronous).
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Tue, 19 Mar 2019 15:43:14 +0000 (11:43 -0400)]
qemu_hotplug: don't call DetachThisHostDevice for hostdev network devices
Back in the bad old days different device types required a different
qemu monitor call to detach them, and so an <interface type='hostdev'>
needed to call the function for detaching hostdevs, while other
<interface> types could be deleted as netdevs.
Times have changed, and *all* device types are detached by calling the
common function qemuDomainDeleteDevice(vm, alias), so we don't need to
differentiate between hostdev interfaces and the others for that
reason.
There are a few other netdev-specific functions called during
qemuDomainDetachNetDevice() (clearing bandwidth limits, stopping the
interface), but those turn into NOPs when type=hostdev, so they're
safe to call for type=hostdev.
The only thing that is different + not a NOP is the call to
virDomainAudit*() when qemuDomainDeleteDevice() fails, so if we add a
conditional for that small bit of code, we can eliminate the callout
from qemuDomainDetachNetDevice() to qemuDomainDetachThisDevice(),
which makes this function fit the desired pattern for merging with the
other detach functions, and paves the way to simplifying
qemuDomainDetachHostDevice() too.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Mon, 18 Mar 2019 22:14:11 +0000 (18:14 -0400)]
qemu_hotplug: refactor qemuDomainDetachDiskLive and qemuDomainDetachDiskDevice
qemuDomainDetachDiskDevice() is only called from one place. Moving the
contents of the function to that place makes
qemuDomainDetachDiskLive() more similar to the other Detach functions
called by the toplevel qemuDomainDetachDevice().
The goal is to make each of the device-type-specific functions do this:
1) find the exact device
2) do any device-specific validation
3) do general validation
4) do device-specific shutdown (only needed for net devices)
5) do the common block of code to send device_del to qemu, then
optionally wait for a corresponding DEVICE_DELETED event from
qemu.
with the final aim being that only items 1 & 2 will remain in each
device-type-specific function, while 3 & 5 (which are the same for
almost every type) will be de-duplicated and moved to the toplevel
function that calls all of these (qemuDomainDetachDeviceLive(), which
will also contain a callout to the one instance of (4) (netdev).
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
Laine Stump [Mon, 18 Mar 2019 20:41:03 +0000 (16:41 -0400)]
qemu_hotplug: eliminate unnecessary call to qemuDomainDetachNetDevice()
qemuDomainDetachHostDevice() has a check at the end that calls
qemuDomainDetachNetDevice() in the case that the hostdev is actually a
Net device of type='hostdev'. A long time ago when device removal was
(supposedly but not actually) synchronous, this would cause some extra
code to be run prior to removing the device (e.g. restoring the original MAC
address of the device, undoing some sort of virtual port profile, etc).
For quite awhile now the device removal has been asynchronous, so that
"extra teardown" isn't handled by the detach function, but instead is
handled by the Remove function called at a later time. The result is
that when we call qemuDomainDetachNetDevice() from
qemuDomainDetachHostDevice(), it ends up just calling
qemuDomainDetachThisHostDevice() and returning, which is exactly what
we do for all other hostdevs anyway.
Based on that, remove the behavioral difference when parent.type ==
VIR_DOMAIN_DEVICE_NET, and just call qemuDomainDetachThisHostDevice()
for all hostdevs.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com>
There are separate Detach functions for PCI, USB, SCSI, Vhost, and
Mediated hostdevs, but the functions are all 100% the same code,
except that the PCI function checks for the guest side of the device
being a PCI Multifunction device, while the other 4 check that the
device's alias != NULL.
The check for multifunction PCI devices should be done for *all*
devices that are connected to the PCI bus in the guest, not just PCI
hostdevs, and qemuIsMultiFunctionDevice() conveniently returns false
if the queried device doesn't connect with PCI, so it is safe to make
this check for all hostdev devices. (It also needs to be done for many
other device types, but that will be addressed in a future patch).
Likewise, since all hostdevs are detached by calling
qemuDomainDeleteDevice(), which requires the device's alias, checking
for a valid alias is a reasonable thing for PCI hostdevs too.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Laine Stump [Tue, 19 Mar 2019 22:57:45 +0000 (18:57 -0400)]
qemu_hotplug: remove unnecessary check for valid PCI address
When support for hotplug/unplug of SCSI controllers was added way back
in December 2009 (commit da9d937b), unplug was handled by calling the
now-extinct function qemuMonitorRemovePCIDevice(), which required a
PCI address as an argument. At the same time, the idea of every device
in the config having a PCI address apparently was not yet fully
implemented, because the author of the patch including a check for a
valid PCI address in the device object.
These days, all PCI devices are guaranteed to have a valid PCI
address. But more important than that, we no longer detach devices by
PCI address, but instead use qemuDomainDeleteDevice(), which
identifies the device by its alias. So checking for a valid PCI
address is just pointless extra code that obscures the high level of
similarity between all the individual qemuDomainDetach*Device()
functions.
Signed-off-by: Laine Stump <laine@laine.org> ACKed-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com>
Laine Stump [Tue, 19 Mar 2019 22:55:15 +0000 (18:55 -0400)]
qemu_hotplug: remove another erroneous qemuDomainDetachExtensionDevice() call
qemuDomainRemoveRNGDevice() calls qemuDomainDetachExtensionDevice().
According to commit 1d1e264f1 that added this code, it should not be
necessary to explicitly remove the zPCI extension device for a PCI
device during unplug, because "QEMU implements an unplug callback
which will unplug both PCI and zPCI device in a cascaded way". In
fact, no other devices call qemuDomainDetachExtensionDevice() during
their qemuDomainRemove*Device() function, so it should be removed from
qemuDomainRemoveRNGDevice as well.
Signed-off-by: Laine Stump <laine@laine.org> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Laine Stump [Tue, 19 Mar 2019 22:51:09 +0000 (18:51 -0400)]
qemu_hotplug: remove erroneous call to qemuDomainDetachExtensionDevice()
qemuDomainDetachControllerDevice() calls
qemuDomainDetachExtensionDevice() when the controller type is
PCI. This is incorrect in multiple ways:
* Any code that tears down a device should be in the
qemuDomainRemove*Device() function (which is called after libvirt
gets a DEVICE_DELETED event from qemu indicating that the guest is
finished with the device on its end. The qemuDomainDetach*Device()
functions should only contain code that ensures the requested
operation is valid, and sends the command to qemu to initiate the
unplug.
* qemuDomainDetachExtensionDevice() is a function that applies to
devices that plug into a PCI slot, *not* necessarily PCI controllers
(which is what's being checked in the offending code). The proper
way to check for this would be to see if the DeviceInfo for the
controller device had a PCI address, not to check if the controller
is a PCI controller (the code being removed was doing the latter).
* According to commit 1d1e264f1 that added this code (and other
support for hotplugging zPCI devices on s390), it's not necessary to
explicitly detach the zPCI device when unplugging a PCI device. To
quote:
There's no need to implement hot unplug for zPCI as QEMU
implements an unplug callback which will unplug both PCI and
zPCI device in a cascaded way.
and the evidence bears this out - all the other uses of
qemuDomainDetachExtensionDevice() (except one, which I believe is
also in error, and is being removed in a separate patch) are only to
remove the zPCI extension device in cases where it was successfully
added, but there was some other failure later in the hotplug process
(so there was no regular PCI device to remove and trigger removal of
the zPCI extension device).
* PCI controllers are not hot pluggable, so this is dead code
anyway. (The only controllers that can currently be
hotplugged/unplugged are SCSI controllers).
Signed-off-by: Laine Stump <laine@laine.org> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Michal Privoznik [Mon, 11 Mar 2019 12:09:52 +0000 (13:09 +0100)]
news: Document kernel requirements for virtual networks
After 7431b3eb9a05068e4b libvirt requires "filter", "nat" and
"mangle" tables to exist for both IPv4 and IPv6. This fact was
missed in the news.xml and since we don't have any better place
to advertise that let's update old news.
This was refined in 686803a1a2e and since that is not released
yet create a new entry documenting the refinement.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Eric Blake [Fri, 22 Mar 2019 22:23:09 +0000 (17:23 -0500)]
snapshot: Add tests of virsh -c test:///default snapshot*
Had this been in place earlier, I would have avoided the bugs in
commit 0baf6945 and 55c2ab3e. Writing the test required me to extend
the power of virsh - creating enough snapshots to cause fanout
requires enough input in a single session that adding comments and
markers makes it easier to check that output is correct. It's still a
bit odd that with test:///default, reverting to a snapshot changes the
domain from running to paused (possibly a bug in how the test driver
copied from the qemu driver) - but the important part is that the test
is reproducible, and any future tweaks we make to snapshot code have
less chance of breaking successful command sequences.
Signed-off-by: Eric Blake <eblake@redhat.com> Acked-by: Michal Privoznik <mprivozn@redhat.com>
Eric Blake [Sun, 24 Mar 2019 00:02:13 +0000 (19:02 -0500)]
virsh: Add 'echo --err' option
Since test:///default resets state on every connection, writing a test
that covers a sequence of commands must be done from a single
session. But if the test wants to exercise particular failure modes as
well as successes, it can be nice to leave witnesses in the stderr
stream immediately before and after the spot where the expected error
should be, to ensure the rest of the script is not causing errors.
Do this by adding an --err option.
Signed-off-by: Eric Blake <eblake@redhat.com> Acked-by: Michal Privoznik <mprivozn@redhat.com>
Eric Blake [Fri, 22 Mar 2019 19:02:39 +0000 (14:02 -0500)]
virsh: Treat any command name starting with # as comment
As the previous commit mentioned, argv mode (such as when you feed
virsh via stdin with <<\EOF instead of via a single shell argument)
didn't permit comments. Do this by treating any command name token
that starts with # as a comment which silently eats all remaining
arguments to the next newline or semicolon.
Note that batch mode recognizes unquoted # at the start of any word as
a command as part of the tokenizer, while this patch only treats # at
the start of the command word as a comment (any other # remaining by
the time vshCommandParse() is processing things was already quoted
during the tokenzier, and as such was probably intended as the actual
argument to the command word earlier in the line).
Now I can do something like:
$ virsh -c test:///default <<EOF
# setup
snapshot-create-as test s1
snapshot-create-as test s2
# check
snapshot-list test --name
EOF
Signed-off-by: Eric Blake <eblake@redhat.com> Acked-by: Michal Privoznik <mprivozn@redhat.com>
Eric Blake [Sat, 23 Mar 2019 22:04:43 +0000 (17:04 -0500)]
snapshot: Avoid infloop during REDEFINE
Commit 55c2ab3e accidentally introduced an infinite loop while
checking whether a redefined snapshot would cause an infinite loop in
chasing its parents back to a root. Alas, 'make check' did not catch
it, so my next patch will be a testsuite improvement that would have
hung and prevented the bug from being checked in to begin with.
Signed-off-by: Eric Blake <eblake@redhat.com> Acked-by: Michal Privoznik <mprivozn@redhat.com>
What happens is that the existing guest disk will receive the ID
'scsi0-0-0-0' due to how Libvirt calculate the alias based on
the address in qemu_alias.c, qemuAssignDeviceDiskAlias. When hotplugging
a disk that happens to have the same address, Libvirt will calculate
the same ID to it and attempt to device_add. QEMU will refuse it:
$ virsh attach-device ub1810 hp-disk-dup.xml
error: Failed to attach device from hp-disk-dup.xml
error: internal error: unable to execute QEMU command 'device_add': Duplicate ID 'scsi0-0-0-0' for device
And Libvirt follows it up with a cleanup code in qemuDomainAttachDiskGeneric
that ends up removing what supposedly is a faulty hotplugged disk but, in
this case, ends up being the original guest disk.
This patch adds an address verification for all attached devices, avoid
calling the driver attach() function using a device with duplicated address.
The change is done in virDomainDefCompatibleDevice when @action is equal
to VIR_DOMAIN_DEVICE_ACTION_ATTACH. The affected callers are:
- qemuDomainAttachDeviceLiveAndConfig, both LIVE and CONFIG cases;
- lxcDomainAttachDeviceFlags, both LIVE and CONFIG.
The check is done using the virDomainDefHasDeviceAddress, a generic
function that can check address duplicates for all supported device
types, not limiting just to DeviceDisk type.
After this patch, this is the result of the previous attach-device call:
$ ./run tools/virsh attach-device ub1810 hp-disk-dup.xml
error: Failed to attach device from hp-disk-dup.xml
error: Requested operation is not valid: Domain already contains a device with the same address
Reported-by: Srikanth Aithal <bssrikanth@in.ibm.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
projects / libvirt.git / log