Commit Graph

32739 Commits

Author SHA1 Message Date
Ján Tomko
080ebb2371 virsh-completer: fix typo
Use the posessive determiner instead of a contracted auxiliary.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
2019-04-03 10:07:36 +02:00
Ján Tomko
912513ca7a virsh: fix indentation of info_managed_save_edit
Use four spaces instead of three.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
2019-04-03 10:07:36 +02:00
Ján Tomko
e5794c542b qemuDomainDiskChangeSupported: use CHECK_STREQ_NULLABLE more
Convert the other string comparisons to use the macro.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
2019-04-03 09:52:54 +02:00
Andrea Bolognani
35d76db2b7 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 ca1471622d 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

   2) Storage backing chain 2 ...
  Offset 0
  Expect [chain member: 0
  path:/home/abologna/src/upstream/libvirt/tests/virstoragedata/raw
  backingStoreRaw: <null>
  capacity: 0
  encryption: 0
  relPath:<null>
  type:1
  format:1
  protocol:none
  hostname:<null>
  ]
  Actual [chain member: 0
  path:/home/abologna/src/libvirt/tests/virstoragedata/raw
  backingStoreRaw: <null>
  capacity: 0
  encryption: 0
  relPath:<null>
  type:1
  format:1
  protocol:none
  hostname:<null>
  ]
                              ... FAILED

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>
2019-04-03 09:46:51 +02:00
Andrea Bolognani
e398f5d47a maint: Update references to ChangeLog*
The files no longer exist, at least not in their previous form,
so references to them need to be reworked to still make sense.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-04-03 09:46:02 +02:00
Andrea Bolognani
be819ebece maint: Drop ChangeLog-old
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>
2019-04-03 09:45:58 +02:00
Andrea Bolognani
ce97c33a79 maint: Stop generating ChangeLog from git
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>
2019-04-03 09:45:25 +02:00
Andrea Bolognani
241a0e8c8b maint: Post-release version bump to 5.3.0
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
2019-04-03 09:44:33 +02:00
Daniel Veillard
7966be03bd Release of libvirt-5.2.0
* docs/news.xml: updated for release date

Signed-off-by: Daniel Veillard <veillard@redhat.com>
2019-04-03 09:35:40 +02:00
intrigeri
80e83d63dc apparmor: support more QEMU architectures
Add hppa, nios2, or1k, riscv32 and riscv64 to the profile.

Fixes: https://bugs.debian.org/914940

Signed-off-by: intrigeri <intrigeri@boum.org>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
2019-04-01 12:32:55 +02:00
Daniel P. Berrangé
ff6440136c apps: remove VM Manager android app
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>
2019-04-01 11:14:55 +01:00
Daniel P. Berrangé
8ad592f016 apps: drop link for zenoss software
The page we link to is a 404 and github repo hasn't been touched since
2012 so is clearly dead.

Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-04-01 11:14:50 +01:00
Daniel P. Berrangé
0647c3d3ac apps: update link for buildbot
The libvirt specific page linked for buildbot is a 404. This replacement
link is the closest to what was originally linked.

Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-04-01 11:14:45 +01:00
Daniel P. Berrangé
b6dde413d3 apps: remove dead archipel project
The project website http://archipelproject.org/ is dead, reporting a
cloudflare error message

The git repo at https://github.com/ArchipelProject/Archipel/ hasn't
had a commit since Nov 2016, and the last release was a beta6 release
in 2013.

Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
2019-04-01 11:14:42 +01:00
Andrea Bolognani
bc4d4cb8f8 news: Update for 5.2.0 release
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
2019-04-01 08:34:02 +02:00
John Ferlan
dab3abfcf5 tools: Tweak wording for iothreadset
Update the wording to note the values for polling are purely dynamic
and won't be saved across domain stop/(re)start or save/restore.

Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
2019-03-30 07:34:34 -04:00
Ján Tomko
4fbc8ddcd0 qemu: error out on attempt to change blkiotune group name
Check that the attribute is the same in qemuDomainDiskChangeSupported
in case somebody tries to change it using the UpdateDevice API.

https://bugzilla.redhat.com/show_bug.cgi?id=1601677

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
2019-03-29 12:54:41 +01:00
Ján Tomko
8535a298a2 qemu: introduce CHECK_STREQ_NULLABLE in qemuDomainDiskChangeSupported
A macro for comparing string fields of the disk.

https://bugzilla.redhat.com/show_bug.cgi?id=1601677

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
2019-03-29 12:54:41 +01:00
Ján Tomko
21a9cb986c Revert "qemu: emit error when trying to update blkiotune group_name in qemuDomainChangeDiskLive"
https://bugzilla.redhat.com/show_bug.cgi?id=1601677

This reverts commit 047cfb05ee
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>
2019-03-29 12:54:41 +01:00
Eric Blake
24b092c404 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>
2019-03-28 12:40:38 -05:00
Eric Blake
83b1808ca2 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: ced0898f86
Reported-by: Bjoern Walk <bwalk@linux.ibm.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-28 10:38:11 -05:00
Laine Stump
3f7cba3f5e 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.

Resolves: https://bugzilla.redhat.com/1683175

Signed-off-by: Laine Stump <laine@laine.org>
2019-03-28 11:19:03 -04:00
Peter Krempa
54eb3e096b qemu: address: Stop reporting warning when USB address can't be released
The warning is reported at a code path which already reports a proper
error so it's pointless to add yet another line into logs.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
2019-03-28 13:40:01 +01:00
Peter Krempa
3c0b1cfdf6 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.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
2019-03-28 13:38:57 +01:00
Peter Krempa
dbd15d6c45 qemu: hotplug: Don't release USB address twice when removing disk
qemuDomainRemoveDiskDevice calls qemuDomainReleaseDeviceAddress which
already calls virDomainUSBAddressRelease so we don't need to call it
again.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
2019-03-28 13:33:50 +01:00
Michal Privoznik
97b729effe qemuxml2argvtest: Drop dependency between testInfoArgName and virQEMUCapsFlags enums
Introduced in fdf6c89ee7, 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>
2019-03-28 09:54:23 +01:00
Michal Privoznik
087a74e160 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>
2019-03-28 09:54:23 +01:00
Daniel P. Berrangé
598641f460 tests: don't abort in fopen(/proc/mounts)
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>
2019-03-27 16:11:34 +00:00
Eric Blake
a6d822cee3 Revert "snapshot: Allow NULL to virDomainSnapshotObjGetDef"
This reverts commit 6b90a84738.

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.

Signed-off-by: Eric Blake <eblake@redhat.com>
2019-03-27 09:21:45 -05:00
Eric Blake
a487890d37 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>
2019-03-27 08:16:10 -05:00
Eric Blake
6b90a84738 snapshot: Allow NULL to virDomainSnapshotObjGetDef
Doing so can simplify some callers.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-27 08:14:42 -05:00
Eric Blake
3d7c683a27 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>
2019-03-27 08:13:24 -05:00
Jiri Denemark
0bc965342b news: Document parallel migration
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
2019-03-27 12:38:44 +01:00
Andrea Bolognani
d2d875fd2f 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 70b4f0e719, 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>
2019-03-27 10:59:54 +01:00
Jiri Denemark
5a303994ff virsh: Add options for parallel migration
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
2019-03-27 09:42:50 +01:00
Jiri Denemark
d3ea986af2 qemu: Add support for parallel migration
The VIR_MIGRATE_PARALLEL flag is implemented using QEMU's multifd
migration capability and the corresponding multifd-channels migration
parameter.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
2019-03-27 09:37:55 +01:00
Jiri Denemark
78be51b3e5 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>
2019-03-27 09:33:51 +01:00
Eric Blake
421861824c 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>
2019-03-26 15:40:57 -05:00
Eric Blake
1c6b6c0ba1 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>
2019-03-26 15:33:07 -05:00
Eric Blake
0b4fac6afd Revert "snapshot: Add virDomainSnapshotObjListFormat"
This reverts commit 86c0ed6f70, 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>
2019-03-26 15:07:47 -05:00
Eric Blake
57d252c740 Revert "snapshot: Add virDomainSnapshotObjListParse"
This reverts commit 1b57269cbc, 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>
2019-03-26 15:07:02 -05:00
Andrea Bolognani
e5e23e3fb9 m4: Add warning when running QEMU as root
Running QEMU as root is a pretty bad idea, so try to make the
user aware of that as part of the configure summary.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-26 18:30:26 +01:00
Andrea Bolognani
29cd1877ac 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>
2019-03-26 18:30:24 +01:00
Michal Privoznik
c34b3eefdf qemufirmwaretest: Produce better message on error
If qemuFirmwareFetchConfigs() returned more or fewer paths than
expected all that we see is the following error message:

  Expected 5 paths, got 7

While it is technically correct (the best kind of correct), we
can do better:

  Unexpected path (i=0). Expected /some/path got /some/other/path

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
2019-03-26 16:43:48 +01:00
Laine Stump
34086fc59e 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>
2019-03-26 11:05:04 -04:00
Laine Stump
78b03a7770 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).

Resolves: https://bugzilla.redhat.com/1658198

Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
2019-03-26 11:05:04 -04:00
Laine Stump
dd60bd62d3 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>
2019-03-26 11:05:04 -04:00
Laine Stump
444c5e7c43 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>
2019-03-26 11:05:04 -04:00
Laine Stump
b914e0eca3 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>
2019-03-26 11:05:04 -04:00
Laine Stump
e1949c7045 qemu_hotplug: rename Chr and Lease Detach functions
qemuDomainDetachDeviceChr and qemuDomainDetachDeviceLease are more
consistent with each other.

Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
2019-03-26 11:05:03 -04:00