Commit Graph

32633 Commits

Author SHA1 Message Date
Cole Robinson
91e85d8939 schemas: domcaps: Make more elements optional
Upcoming changes will make outputting these subelements optional.

While we are here drop the useless interleave: since this is an output
only format the elements are always in the same order

Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Cole Robinson
9aac3da9b0 bhyve: domcaps: fill in explicit supported BOOL_NO
<hostdev> and <features> are not supported. <loader>, <graphics>,
and <video> are supported conditionally

Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Cole Robinson
697fb8a381 libxl: domcaps: fill in explicit supported BOOL_NO
None of the <feature> bits are supported, and the <loader> piece
is only conditionally supported

Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Cole Robinson
cd35c4af60 qemu: domcaps: fill in explicit supported BOOL_NO
Only gic->supported needs an explicit BOOL_NO setting, all other
'supported' values are handling things correctly

Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Cole Robinson
871093b6a3 conf: domcaps: use virTristateBool for 'supported'
Switch most 'supported' handling to use virTristateBool, so eventually
we can handle the ABSENT state.

For now the XML formatter treats ABSENT the same as FALSE, so there's
no functional output change. This will be addressed in later patches

Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Cole Robinson
bf68454c46 conf: domcaps: Add single line formatting macro
Similar to the macros we have for formatting enums, add a macro to
simplify formatting the pattern:

  <FOO supported='yes|no'/>

Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Cole Robinson
ad12932481 tests: domcaps: Remove 'full' test
The 'full' test verifies the output of a virDomainCapsPtr built
by hand. It has the following problems:

The domcaps test suite nowadays has 3 hypervisor driver implementations
which should give us plenty of opportunity to get full domcaps coverage.
I don't think this test has much value. And it has the following issues:

- Requires manual intervention to test new domcaps XML, which is easy
  to miss, for example gic bits aren't covered there.
- The SET_ALL_BITS trick it uses to fill in enums will output
  values that are never reported by any driver implementation
  (strings like 'default')

Let's remove it

Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Cole Robinson
0b89ef5596 tests: domcaps: Remove unused typedef
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Cole Robinson
5c11e46065 tests: domcaps: Add a default 'empty' test
The 'empty' demonstrates XML generated when only bare minimum caps
data has been filled in. This will demonstrate changes that alter
the default XML output.

Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 10:51:02 -04:00
Andrea Bolognani
c2195bee24 tests: Document how to add new replies files
We already document how to generate them, so might as well
go the extra mile and document the remaining steps.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
2019-03-18 15:49:47 +01:00
Michal Privoznik
ed454facd4 node_device_hal.c: Follow _class -> klass rename
In 0eca80e60 _class was renamed to klass for variety of struct
members. However, gather_usb_cap() was missed out in this rename
leaving FreeBSD build broken.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
2019-03-18 14:46:52 +01:00
Cole Robinson
c53acd2ad1 Drop needless virtType validation
This code originates from:

commit d0aa10fdd6
Author: Daniel P. Berrange <berrange@redhat.com>
Date:   Tue Mar 3 12:03:44 2009 +0000

    QEMU security driver usage for sVirt support (James Morris, Dan Walsh, Daniel Berrange)

Originally in the qemudDomainGetSecurityLabel function. It doesn't
appear to have done anything useful back then either. The other two
instances look like copy+paste

Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
2019-03-18 09:35:00 -04:00
Michal Privoznik
2e556e00ca storageVolWipePattern: Don't take shortcut to refreshPool()
In d16f803d78 we've tried to solve an issue that after wiping an
image its format might have changed (e.g. from qcow2 to raw) but
libvirt wasn't probing the image format. We fixed this by calling
virStorageBackendRefreshVolTargetUpdate() which is what
refreshPool() would end up calling. But this shortcut is not good
enough because the function is called only for local types of
volumes (like dir, fs, netfs). But now that more backends support
volume wiping we have to call the function with more caution.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2019-03-18 13:20:36 +01:00
Michal Privoznik
f7b9d6f78b storage_backend_iscsi_direct: Simplify vol zeroing
So far we have two branches: either we zero BLOCK_PER_PACKET
(currently 128) block at once, or if we're close to the last block
then we zero out one block at the time. This is very suboptimal.
We know how many block are there left. Might as well just write
them all at once.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2019-03-18 13:20:36 +01:00
Peter Krempa
61d1abd5f1 virsh: man: Document asynchronous behaviour of detach-device-alias
This command is fully async. Note that users can use virsh event to be
notified of the guest actually removing the device.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
2019-03-18 13:11:29 +01:00
Peter Krempa
e95a66349a virsh: man: Document quirks of device-detach and friends
Mention that successful return does not equal to device being detached
similarly as we do at the API level.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
2019-03-18 13:11:29 +01:00
Nikolay Shirokovskiy
bb1f41c075 news: update for new class element of PCI nodedev capability
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-18 11:15:02 +03:00
Nikolay Shirokovskiy
3bd4ed4630 xml: nodedev: add class info for pci capability
This info can be useful to filter devices visible
to mgmt clients so that they won't see devices that
unsafe/not meaningful to pass thru.

Provide class info the way it is provided by udev or
kernel that is as single 6-digit hexadecimal.

Class element is not optional. I guess this should not
break users that use virNodeDeviceCreateXML because
they probably specify only scsi_host capability on
input and then node device driver gets other capabilities
from udev after device appeared.

HAL driver does not get support for the new element in
this patch.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-18 11:14:58 +03:00
Nikolay Shirokovskiy
0eca80e606 conf: don't use "class" as name
Vim treats *.h files as cpp ones with respect to syntax highlighting.
Thus "class" in _virNodeDevCapPCIDev highlighted mistakenly.
This can be fixed by filetype detection code tunables but it
is more convinient to skip this tuning by every project member.

Let's just use "klass" as field name instead of _class or class
and add syntax rule.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-18 11:14:37 +03:00
Nikolay Shirokovskiy
e7e6861489 vz: build fix for virdomainsnapshotobjlist.h
Commit [1] moved snapshot list functions declaration into
its own file but missed a fix for vz driver.

[1] 9b75154c : snapshot: Break out virDomainSnapshotObjList into its own file

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
2019-03-18 11:07:23 +03:00
Michal Privoznik
d8b2ec2149 virsh-pool: Offer only active pool for pool-refresh completer
Only active pools can be refreshed. But our completer offers just
all pool, even inactive ones.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2019-03-16 07:50:51 +01:00
Michal Privoznik
ccc7ffb4ef storagePoolRefreshFailCleanup: Clear volumes on failed refresh
If pool refresh failed, then the internal table of volumes is
probably left in inconsistent or incomplete state anyways. Clear
it out then. This has an advantage that we can move the
virStoragePoolObjClearVols() from those very few backends that
do call it.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2019-03-16 07:50:51 +01:00
Michal Privoznik
bd45cedbe5 storage_driver: Introduce storagePoolRefreshImpl()
This is a wrapper over refreshPool() call as at all places we are
doing basically the same. Might as well have a single function to
call.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2019-03-16 07:50:51 +01:00
Michal Privoznik
f8aecea779 virISCSIDirectReportLuns: Drop ClearVols
In bf5cf610f2 I've fixed a problem where iscsi-direct
backend was reporting only the last LUN. The fix consisted of
moving virStoragePoolObjClearVols() one level up. However, as it
turns out, storage driver already calls it before calling
refreshPool callback (which is
virStorageBackendISCSIDirectRefreshPool() in this case).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2019-03-16 07:50:51 +01:00
Michal Privoznik
9bf194c23f iscsi_direct: Don't overwrite error in virStorageBackenISCSIDirectWipeVol()
If virStorageBackendISCSIDirectVolWipeZero() fails, it has
already reported an error which is probably specific enough. Do
not overwrite it with some generic one.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2019-03-16 07:50:51 +01:00
Michal Privoznik
96e5c4177e iscsi_direct: Make virStorageBackendISCSIDirectGetLun report error properly
This function reports error for one of the two error paths. This
is unfortunate as a caller see this function failing but doesn't
know right away if an error was reported.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
2019-03-16 07:50:51 +01:00
Jim Fehlig
25e2e4e04f news: Add entry describing support for Xen's max grant frames
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 15:53:51 -06:00
Andrea Bolognani
f703b487ad news: Document dropping support for certain init scripts
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 18:36:19 +01:00
Andrea Bolognani
cf6b65a19e m4: Simplify LIBVIRT_CHECK_INIT_SCRIPT
Now that we no longer support the weird "redhat+systemd"
configuration, we can make our code slightly simpler.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 18:36:19 +01:00
Andrea Bolognani
912fe2df9d Drop support for "Red Hat" init scripts
Despite the misleading name, these were supposed to be used
with a System V style init; however, none of the platforms we
target is using that kind of init anymore: almost all Linux
distributions have switched to systemd, those that haven't
(such as Gentoo and Alpine) are mostly using OpenRC with
custom init scripts, and the BSDs have been doing their own
thing all along.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 18:36:19 +01:00
Andrea Bolognani
b8cfdee42b Drop support for Upstart init scripts
Not a single one of the platforms we target still uses Upstart, and
the Upstart project itself has been abandoned for several years now.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 18:36:19 +01:00
Andrea Bolognani
6849abb835 travis: Drop $DISTCHECK_CONFIGURE_FLAGS
It's no longer used.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 18:36:19 +01:00
Andrea Bolognani
16210b1de4 travis: Stop using --with-init-script
We're gonna drop support for non-systemd init scripts soon,
and we don't want Travis CI builds to break when we do.

Since we have init system auto-detection, we can just rely on
that and stop passing --with-init-script to configure entirely.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 18:36:19 +01:00
Eric Blake
9b75154c07 snapshot: Break out virDomainSnapshotObjList into its own file
snapshot_conf.h was mixing three separate types: the snapshot
definition, the snapshot object, and the snapshot object list.
Separate out the snapshot object list code into its own file, and
update includes for affected clients.

This is just code motion, but done in preparation of sharing a lot of
the object list code with checkpoints.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 11:43:09 -05:00
Eric Blake
21b2651e72 snapshot: Export two functions prior to file split
The next patch will require access to the helper functions
virDomainSnapshotDefFormatInternal and
virDomainSnapshotRedefineValidate from two different files; make the
file split easier by exporting these functions.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 11:37:59 -05:00
Eric Blake
ca20690e9f snapshot: Break out virDomainSnapshotObj into its own file
snapshot_conf.h was mixing three separate types: the snapshot
definition, the snapshot object, and the snapshot object list.
Separate out the snapshot object code into its own file, which
includes moving a typedef to avoid circular inclusions.

Mostly straight code motion, although I fixed a comment along
the way, now that virDomainSnapshotForEachDescendent now
guarantees a topological visit (missed in b647d219).

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 11:34:32 -05:00
Eric Blake
41e3d35e09 snapshot: Sort virconftypes.h
It's easier to locate a typedef if they are stored in sorted order;
do so mechanically via:

$ sed -i '/typedef struct/ {N; N; s/\n//g}' src/conf/virconftypes.h
$ # sorting the lines
$ sed -i '/typedef struct/ s/;/;\n/g' src/conf/virconftypes.h

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 11:33:52 -05:00
Eric Blake
23c15e34c3 conf: Split capabilities forward typedefs into virconftypes.h
As explained in the previous patch, collecting pointer typedefs into a
common header makes it easier to avoid circular inclusions.  Continue
the efforts by pulling the appropriate typedefs from capabilities.h
into the new header.

This patch is just straight code motion (all typedefs are listed in
the same order before and after the patch); a later patch will sort
things for legibility.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 11:31:12 -05:00
Eric Blake
c555ec2416 conf: Split domain forward typedefs into virconftypes.h
Right now, snapshot_conf.h is rather large - it deals with three
separate types: virDomainSnapshotDef (the snapshot definition as it
maps to XML), virDomainSnapshotObj (an object containing a def and the
relationship to other snapshots), and virDomainSnapshotObjList (a list
of snapshot objects), where two of the three types are currently
public rather than opaque.  What's more, the types are circular: a
snapshot def includes a virDomainPtr, which contains a snapshot list,
which includes a snapshot object, which includes a snapshot def.

In order to split the three objects into separate files, while still
allowing each header to use sane typedefs to incomplete pointers, the
obvious solution is to lift the typedefs into yet another header, with
no other dependencies.  Start the split by factoring out all struct
typedefs from domain_conf.h (enum typedefs don't get used in function
signatures, and function typedefs tend not to suffer from circular
referencing, so those stay put).  The only other exception is
virDomainStateReason, which is only ever used directly rather than via
a pointer.

This patch is just straight code motion (all typedefs are listed in
the same order before and after the patch); a later patch will sort
things for legibility.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 11:29:06 -05:00
Eric Blake
1c560052a8 object: Add sanity check on correct parent class
Checking that the derived class is larger than the requested parent
class saves us from some obvious mistakes, but as written, it does not
catch all the cases; in particular, it is easy to forget to update a
VIR_CLASS_NEW when changing the 'parent' member from virObject to
virObjectLockabale, but where the size checks don't catch that.  Add a
parameter for one more layer of sanity checking.

It would be cool if we could get gcc to stringize typeof(parent) into
the string name of that type, so that we could confirm that the
precise parent class is in use rather than just a struct that happens
to have the same size as the parent class.  But sizeof checks are
better than nothing.

Note that I did NOT change the fact that we require derived classes to
be larger (as the difference in size makes it easy to tell classes
apart), which means that even if a derived class has no functionality
to add (but rather exists for compiler-enforced type-safety), it must
still include a dummy member.  But I did fix the wording of the error
message to match the code.

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-15 11:10:12 -05:00
Erik Skultety
75a9169881 qemu: command: Override HOME variable for system QEMU
By default, qemu user's home dir points to '/' which shouldn't be used
at all. We therefore pass the HOME variable from the current variable
iff not running as SUID, which means that for systemd we never set it.
This patch makes sure, that for system QEMU this is always set to
libDir/<driver>, session mode is left untouched.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 16:41:26 +01:00
Erik Skultety
7e73137495 qemu: command: Enforce setting XDG variables for system QEMU
For session mode, only XDG_CACHE_HOME is set, because we want to remain
integrating with services in user session, but for system mode, this
would have become reading/writing to '/' which carries the obvious issue
with permissions (also, '/' is the wrong location in 99.9% cases anyway).

Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 16:41:26 +01:00
Erik Skultety
2d69af2907 util: command: Introduce virCommandAddEnvXDG helper
Some modules/libraries within QEMU could make use of the XDG_ vars when
writing their data to the disk. Define the most common XDG variables
and point them to the specific driver's libDir, i.e.

XDG_CACHE_HOME -> /var/lib/libvirt/<driver>/.cache
XDG_DATA_HOME -> /var/lib/libvirt/<driver>/.local/share
XDG_CONFIG_HOME -> /var/lib/libvirt/<driver>/.config

Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 16:41:26 +01:00
Peter Krempa
0b7d544c88 qemu: hotplug: Merge virtio and non-virtio disk unplug code
The functions do basically exactly the same thing modulo few checks.
In case of virtio disks we check that the device is not multifunction as
that can't be unplugged at once. In case of USB and SCSI disks we
checked that no active block job is running.

The check for running blockjobs should have also been done for virtio
disks. By moving the multifunction check into the common function we fix
this case and also simplify the code.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 16:11:20 +01:00
Peter Krempa
eb437cfdf8 qemu: hotplug: Use switch statement for selecting disk bus function
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 16:11:20 +01:00
Peter Krempa
afa15d78cb qemu: hotplug: Use typecasted enum in qemuDomainDetachDeviceDiskLive
Use the correct type in switch and populate the missing cases.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 16:11:20 +01:00
Peter Krempa
70d0689812 qemu: hotplug: Remove 'ret' variable in qemuDomainDetachDeviceDiskLive
We don't have any cleanup section, we can return the value directly.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
2019-03-15 16:11:20 +01:00
Eric Blake
a6a25d5cb6 snapshot: More clarification about REDEFINE
Based on recent list questions about the proposed addition of
virDomainCheckpointCreateXML(REDEFINE), it is worth adding some
clarification to the existing snapshot redefine documentation that is
serving as the basis for checkpoints.

Normal snapshot creation requires very few elements from the user XML
(libvirt can pick sane defaults for items that are omitted, and many
fields, including <domain>, are documented as readonly output fields
ignored on input, produced by drivers that track it). But during
REDEFINE, the API wants the complete XML produced by an earlier
virDomainSnapshotGetXMLDesc; as the domain definition has likely
changed since the snapshot was first created, libvirt is unable to
recreate a <domain> sub-element that matches the original output
representing the domain state at the time the snapshot was first
created. In fact, reverting without a <domain> sub-element is risky
enough that we had to add a FORCE flag for virDomainSnapshotRevert().
In short, we only support omitting domain for qemu because of
backwards-compatibility to snapshots created before 0.9.5 started
capturing <domain>; even though there are other drivers like vbox that
do not output <domain> because they have other reliable ways to
revert.

And based on the confusion caused when omitting <domain> from snapshot
XML, the initial design for checkpoints in later patches will make
<domain> a mandatory element during its REDEFINE.

[Side note: the fact that <domain> can appear in <domainsnapshot> is a
reason we cannot add a new API for a bulk listing or redefine of all
snapshots of a single domain in one XML call (for example, a 1M
<domain> XML * 16 snapshots explodes into 16M in a bulk form, which
gets difficult to send over RPC). Perhaps we could add a flag to
request that the <domain> sub-element be omitted on output, but such
output is no longer suitable for sane REDEFINE input.]

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
2019-03-15 08:32:37 -05:00
Eric Blake
632ac8f8e7 virobject: Improve documentation
I had to inspect the code to learn whether a final virObjectUnref()
calls ALL dispose callbacks in child-to-parent order (akin to C++
destructors), or whether I manually had to call a parent-class dispose
when writing a child class dispose method.  The answer is the
former. (Thankfully, since VIR_FREE wipes out pointers for safety,
even if I had guessed wrong, I probably would not have tripped over a
double-free fault when the parent dispose ran for the second time).  I
also had to read the code to learn if a dispose method was even
mandatory (it is not, although getting NULL through VIR_CLASS_NEW
requires a macro).  While at it, the VIR_CLASS_NEW macro requires that
the virObject component at offset 0 be reached through the name
'parent', not 'object'.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
2019-03-15 08:30:47 -05:00
Michal Privoznik
c2bc419131 qemu_hotplug: Fix a rare race condition when detaching a device twice
https://bugzilla.redhat.com/show_bug.cgi?id=1623389

If a device is detached twice from the same domain the following
race condition may happen:

1) The first DetachDevice() call will issue "device_del" on qemu
monitor, but since the DEVICE_DELETED event did not arrive in
time, the API ends claiming "Device detach request sent
successfully".

2) The second DetachDevice() therefore still find the device in
the domain and thus proceeds to detaching it again. It calls
EnterMonitor() and qemuMonitorSend() trying to issue "device_del"
command again. This gets both domain lock and monitor lock
released.

3) At this point, qemu sends us the DEVICE_DELETED event which is
going to be handled by the event loop which ends up calling
qemuDomainSignalDeviceRemoval() to determine who is going to
remove the device from domain definition. Whether it is the
caller that marked the device for removal or whether it is going
to be the event processing thread.

4) Because the device was marked for removal,
qemuDomainSignalDeviceRemoval() returns true, which means the
event is to be processed by the thread that has marked the device
for removal (and is currently still trying to issue "device_del"
command)

5) The thread finally issues the "device_del" command, which
fails (obviously) and therefore it calls
qemuDomainResetDeviceRemoval() to reset the device marking and
quits immediately after, NOT removing any device from the domain
definition.

At this point, the device is still present in the domain
definition but doesn't exist in qemu anymore. Worse, there is no
way to remove it from the domain definition.

Solution is to note down that we've seen the event and if the
second "device_del" fails, not take it as a failure but carry on
with the usual execution.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
2019-03-15 13:45:34 +01:00