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>
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>
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>
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>
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>
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>
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>
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>
qemuDomainDetachDeviceChr and qemuDomainDetachDeviceLease are more
consistent with each other.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
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>
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>
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>
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>
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>
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>
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>
They were added in qemu commit 7572150c189c6553c2448334116ab717680de66d
released in v0.14.0.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
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>
There were two outliers at the end of the file beyond the Vcpu
functions.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
It was sitting down in the middle of all the qemuDomainDetach*()
functions. Move it up with the rest of the qemuDomain*Graphics*()
functions.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
It's now only called from one place, and combining the two functions
highlights the similarity with Detach functions for other device
types.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
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>
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>
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>
Having an InfoPtr named "dev" made my brain hurt. Renaming it to
"info" gives one less thing to confuse when looking at the code.
Signed-off-by: Laine Stump <laine@laine.org>
ACKed-by: Peter Krempa <pkrempa@redhat.com>
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>
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>
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>
Now that the core of SnapshotObj is agnostic to snapshots and can be
shared with upcoming checkpoint code, it is time to rename the struct
and the functions specific to list operations. A later patch will
shuffle which file holds the common code. This is a fairly mechanical
patch.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Another step towards making the object list reusable for both
snapshots and checkpoints: the list code only ever needs items that
are in the common virDomainMomentDef base type. This undoes a lot of
the churn in accessing common members added in the previous patch, and
the bulk of the patch is mechanical. But there was one spot where I
had to unroll a VIR_STEAL_PTR to work around changed types.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Pull out the common parts of virDomainSnapshotDef that will be reused
for virDomainCheckpointDef into a new base class. Adjust all callers
that use the direct fields (some of it is churn that disappears when
the next patch refactors virDomainSnapshotObj; oh well...).
Someday, I hope to switch this type to be a subclass of virObject, but
that requires a more thorough audit of cleanup paths, and besides
minimal incremental changes are easier to review.
As for the choice of naming:
I promised my teenage daughter Evelyn that I'd give her credit for her
contribution to this commit. I asked her "What would be a good name
for a base class for DomainSnapshot and DomainCheckpoint". After
explaining what a base class was (using the classic OOB Square and
Circle inherit from Shape), she came up with "DomainMoment", which is
way better than my initial thought of "DomainPointInTime" or
"DomainPIT".
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
An upcoming patch will rework virDomainSnapshotObjList to be generic
for both snapshots and checkpoints; reduce the churn by adding a new
accessor virDomainSnapshotObjGetDef() which returns the
snapshot-specific definition even when the list is rewritten to
operate only on a base class, then using it at sites that that are
specific to snapshots. Use VIR_STEAL_PTR when appropriate in the
affected lines.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Rather than allowing a leaky abstraction where multiple drivers have
to open-code operations that update the relations in a
virDomainSnapshotObjList, it is better to add accessor functions so
that updates to relations are maintained closer to the internals.
This patch finishes the job started in the previous patch, by getting
rid of all direct access to nchildren, first_child, or sibling outside
of the lowest level functions, making it easier to refactor later on.
The lone new caller to virDomainSnapshotObjListSize() checks for a
return != 0, because it wants to handles errors (-1, only possible if
the hash table wasn't allocated) and existing snapshots (> 0) in the
same manner; we can drop the check for a current snapshot on the
grounds that there shouldn't be one if there are no snapshots.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Rather than allowing a leaky abstraction where multiple drivers have
to open-code operations that update the relations in a
virDomainSnapshotObjList, it is better to add accessor functions so
that updates to relations are maintained closer to the internals.
This patch starts the task with a single new function:
virDomainSnapshotMoveChildren(). The logic might not be immediately
obvious [okay, that's an understatement - the existing code uses black
magic ;-)], so here's an overview: The old code has an implicit for
loop around each call to qemuDomainSnapshotReparentChildren() by using
virDomainSnapshotForEachChild() (you'll need a wider context than
git's default of 3 lines to see that); the new code has a more visible
for loop. Then it helps if you realize that the code is making two
separate changes to each child object: STRDUP of the new parent name
prior to writing XML files (unchanged), and touching up the pointer to
the parent object (refactored); the end result is the same whether a
single pass made both changes (both in driver code), or whether it is
split into two passes making one change each (one in driver code, the
other in the new accessor).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
It is easier to track the current snapshot as part of the list of
snapshots. In particular, doing so lets us guarantee that the current
snapshot is cleared if that snapshot is removed from the list (rather
than depending on the caller to do so, and risking a use-after-free
problem, such as the one recently patched in 1db9d0efbf). This
requires the addition of several new accessor functions, as well as a
useful return type for virDomainSnapshotObjListRemove(). A few error
handling sites that were previously setting vm->current_snapshot =
NULL can now be dropped, because the previous function call has now
done it already. Also, qemuDomainRevertToSnapshot() was setting the
current vm twice, so keep only the one used on the success path.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Rework the logic in qemuDomainSnapshotLoad() to set
vm->current_snapshot only once at the end of the loop, rather than
repeatedly querying it during the loop, to make it easier for the next
patch to use accessor functions rather than direct manipulation of
vm->current_snapshot. When encountering multiple snapshots claiming
to be current (based on the presence of an <active>1</active> element
in the XML, which libvirt only outputs for internal use and not for
any public API), this changes behavior from warning only once and
running with no current snapshot, to instead warning on each duplicate
and selecting the last one encountered (which is arbitrary based on
readdir() ordering, but actually stands a fair chance of being the
most-recently created snapshot whether by timestamp or by the
propensity of humans to name things in ascending order).
Note that the code in question is only run by libvirtd when it first
starts, reading state from disk from the previous run into memory for
this run. Since the data resides somewhere that only libvirt should be
touching (typically /var/lib/libvirt/qemu/snapshot/*), it should be
clean. So in the common case, the code touched here is unreachable.
But if someone is actually messing with files behind libvirt's back,
they deserve the change in behavior.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The only use for the 'current' member of virDomainSnapshotDef was with
the PARSE/FORMAT_INTERNAL flag for controlling an internal-use
<active> element marking whether a particular snapshot definition was
current, and even then, only by the qemu driver on output, and by qemu
and test driver on input. But this duplicates vm->snapshot_current,
and gets in the way of potential simplifications to have qemu store a
single file for all snapshots rather than one file per snapshot. Get
rid of the member by adding a bool* parameter during parse (ignored if
the PARSE_INTERNAL flag is not set), and by adding a new flag during
format (if FORMAT_INTERNAL is set, the value printed in <active>
depends on the new FORMAT_CURRENT).
Then update the qemu driver accordingly, which involves hoisting
assignments to vm->current_snapshot to occur prior to any point where
a snapshot XML file is written (although qemu kept
vm->current_snapshot and snapshot->def_current in sync by the end of
the function, they were not always identical in the middle of
functions, so the shuffling gets a bit interesting). Later patches
will clean up some of that confusing churn to vm->current_snapshot.
Note: even if later patches refactor qemu to no longer use
FORMAT_INTERNAL for output (by storing bulk snapshot XML instead), we
will always need PARSE_INTERNAL for input (because on upgrade, a new
libvirt still has to parse XML left from a previous libvirt).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
And adjust virQEMUCapsSetList to use it. It will also be used in future
patches.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Storage source private data can be parsed along with other components of
private data rather than a separate function which is called from
multiple places.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Set report=true for all enums currently formatted in the XML
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
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>
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>
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>
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>
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>
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>
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>
