Skip actual snapshot creation code if we have 0 disks to snapshot.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All cases taking the 'cleanup' path can take the original 'error' path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Introduce 'rc' for collecting state from monitor commands so that we can
initialize 'ret' to -1. This also fixes few cases which could return 0
from the function despite an error condition.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuDomainSnapshotDiskDataFree also removes the resources associated
with the disk data. Move the unlinking of the just-created file so that
we can unify the cleanup paths.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
In commit cbb4d229de I named the function with 'free' suffix, but at
that time it already did some non-freeing tasks. Rename it to make it
obvious that it's not just memory managemet.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function skips disks which are not selected for snapshot. Rather
than creating a sparse array and check whether the given field is filled
compress the entries.
Note that this does not allocate a smaller array, but the memory
allocation is short-lived.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If there's an offline config definition save it unconditionally even if
it was not modified.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The error path is unlikely thus saving the status XML even if we didn't
modify it does not add much burden.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
After getting rid of pre-transaction qemu support the cleanup section is
unused.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Split out the 'shallow' and 'reuse' flags as booleans rather than passing
in flags and constructing them in irrelevant APIs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
At the point when we want to modify the permissions for the 'mirror' we
know whether it is supposed to have a backing chain or no. Given that
mirror->backingStore is populated only when we'd need to touch it ayways
we can use qemuDomainStorageSourceChainAccessAllow even in place of
qemuDomainStorageSourceAccessAllow used for other cases to simplify the
code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
One code path open-coded qemuDomainStorageSourceChainAccessAllow badly
and also did not integrate with the locking code.
Replace the separate calls with qemuDomainStorageSourceChainAccessAllow
which does everything internally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since 4e797f1a we parse backingStore of mirror which will later be used
with blockdev. Add some validation for the user passed mirror at the
current point to make sure it's not used improperly.
Validate that it's not used without blockdev and also that it's not
passed when not requesting a shallow copy. Also add a chain terminator
for a deep copy since we know the resulting mirror will not have chain.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since 3decae00e9 qemuDomainStorageSourceAccessAllow revokes the
permissions it granted if it fails halfway, thus we can remove some
calls to qemuDomainStorageSourceAccessRevoke which tried to undo this
situation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since 3decae00e9 qemuDomainStorageSourceAccessRevoke keeps the libvirt
error which was set prior to the call around even after the call, thus
we don't need to do the same when reverting access in the block copy
code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
When aborting or pivoting a block job we record which operation we do
for the mirror in the virDomainDiskDef structure. As everything is
synchronized by a job it's not necessary to modify the state prior to
calling the monitor and resetting the state on failure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
All blockjobs get their status updated by events from qemu, so this code
no longer makes sense.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This reverts commit dfd70ca1eb.
Pushed by a mistake, sorry. There's still some discussion going
on upstream.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Qemu added reporting of virtio balloon new statistics stat-htlb-pgalloc and
stat-htlb-pgfail since qemu-3.0 commit b7b12644297. The value of
stat-htlb-pgalloc represents the number of successful hugetlb page allocations
while stat-htlb-pgfail represents the number of failed ones. Add this
statistics reporting to libvirt.
To enable this feature for vm, guest kenel >= 4.17 is required because
the exporting hugetlb page allocation for virtio balloon is introduced
since 6c64fe7f.
Signed-off-by: Han Han <hhan@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Snapshot create operation saves the live XML and uses it to replace the
domain definition in case of revert. But the VM config XML is not saved
and the revert operation does not address this issue. This commit
prevents the config XML from being overridden by snapshot definition.
An active domain stores both current and new definitions. The current
definition (vm->def) stores the live XML and the new definition
(vm->newDef) stores the config XML. In an inactive domain, only the
config XML is persistent, and it's saved in vm->def.
The revert operation uses the virDomainObjAssignDef() to set the
snapshot definition in vm->newDef, if domain is active, or in vm->def
otherwise. But before that, it saves the old value to return to
caller. This return is used here to restore the config XML after
all snapshot startup process finish.
Signed-off-by: Maxiwell S. Garcia <maxiwell@linux.ibm.com>
This function gets snapshot XML (provided by used) as an
argument. It parses it into a local variable @def and then sets
some more members (e.g. it creates a copy of live domain XML).
Then it proceeds to checking if snapshot XML is valid (e.g. it
contains as many disks as currently in the domain). If this fails
then the control jumps to endjob label and subsequently return
from the function. This is where AUTOFREE function for @def is
ran. Well, because the code says to run plain VIR_FREE() we leak
some memory because @def is actually an object and therefore
it should have been declared as AUTOUNREF.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
This brings about a couple of benefits:
- use of VIR_AUTOUNREF() simplifies several callers
- Fixes a todo about virDomainMomentObjList not being polymorphic enough
Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
VIR_CLASS_NEW insists that descendents of virObject have 'parent' as
the name of their inherited base class member at offset 0. While it
would be possible to write a new class-creation macro that takes the
actual field name 'current', and rewrite VIR_CLASS_NEW to call the new
macro with the hard-coded name 'parent', it seems less confusing if
all object code uses similar naming. Thus, this is a mechanical rename
in preparation of making virDomainSnapshotDef a descendent of
virObject.
Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
VIR_CLASS_NEW insists that descendents of virObject have 'parent' as
the name of their inherited base class member at offset 0. While it
would be possible to write a new class-creation macro that takes the
actual field name, and rewrite VIR_CLASS_NEW to call the new macro
with the hard-coded name 'parent', so that we could make
virDomainMomentDef use a custom name for its base class, it seems less
confusing if all object code uses similar naming. Thus, this is a
mechanical rename in preparation of making virDomainSnapshotDef a
descendent of virObject, when we can no longer use 'parent' for a
different purpose than the base class.
Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
After 65a372d6e0 the @cfg variable is no longer used. This means
we can drop it and therefore drop 'cleanup' label with it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
This caused the live XML to report the 'bridge' type instead of the
'network' type, which is a behavioural regression.
It also breaks 'virsh domif-setlink', 'virsh update-device' and
'virsh domiftune'
This reverts commit 518026e159.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1697676
If an user tries to attach a device with colliding user alias
then we attach it happily and thus leave domain unable to start.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
When attaching a device to live XML we don't care (well,
shouldn't care) that there's already a device in inactive XML
that has the same user alias.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
If we're attaching a device to both inactive and live XML then
@ret is overwritten which may result in incorrect return value.
For instance, if attaching to inactive XML succeeds, @ret is
assigned value of zero and control proceeds to attaching the
device to live XML. Here, if say
virDomainDeviceValidateAliasForHotplug() fails the control jumps
over to 'cleanup' label and zero is returned indicating success.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Our coding style specifies that only negative values are considered as
error. Check for return value of virDomainDiskInsert() properly,
following the style. Not that the function can now return anything other
than 0 or -1, but it just triggers my OCD.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
If the current QEMU guest can't wake up from suspend properly,
and we are able to determine that, avoid suspending the guest
at all. To be able to determine this support, QEMU needs to
implement the 'query-current-machine' QMP call. This is reflected
by the QEMU_CAPS_QUERY_CURRENT_MACHINE cap.
If the cap is enabled, a new function qemuDomainProbeQMPCurrentMachine
is called. This is wrapper for qemuMonitorGetCurrentMachineInfo,
where the 'wakeup-suspend-support' flag is retrieved from
'query-current-machine'. If wakeupSuspendSupport is true,
proceed with the regular flow of qemuDomainPMSuspendForDuration.
The absence of QEMU_CAPS_QUERY_CURRENT_MACHINE indicates that
we're dealing with a QEMU version older than 4.0 (which implements
the required QMP API). In this case, proceed as usual with the
suspend logic of qemuDomainPMSuspendForDuration, since we can't
assume whether the guest has support or not.
Fixes: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1759509
Reported-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Ports allocated on virtual networks with type=nat|route|open all get
given an actual type of 'network'.
Only ports in networks with type=bridge use an actual type of 'bridge'.
This distinction makes little sense since the virtualization drivers
will treat both actual types in exactly the same way, as they're all
just bridge devices a VM needs to be connected to.
This doesn't affect user visible XML since the "actual" device XML
is internal only, but we need code to convert the data upgrades.
Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Firstly, VIR_STRDUP() accepts NULL, so there is no need to check
if the string we want to duplicate is not-NULL. Secondly,
virDomainNetSetModelString() also accepts NULL. Thirdly, we have
VIR_AUTOFREE().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
To ease converting the net->model value to an enum, add
the wrapper functions:
virDomainNetGetModelString
virDomainNetSetModelString
virDomainNetStreqModelString
virDomainNetStrcaseeqModelString
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
The port allocation APIs are currently called unconditionally for all
types of NIC, but (mostly) only do anything for NICs with type=network.
The exception is the port allocate API which does some validation even
for NICs with type!=network. Relying on this validation is flawed,
however, since the network driver may not even be installed. IOW virt
drivers must not delegate validation to the network driver for NICs
with type != network.
This change allows us to report errors when the virtual network driver
is not registered.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
My earlier commit be46f61326 was incomplete. It removed caching of
microcode version in the CPU driver, which means the capabilities XML
will see the correct microcode version. But it is also cached in the
QEMU capabilities cache where it is used to detect whether we need to
reprobe QEMU. By missing the second place, the original commit
be46f61326 made the situation even worse since libvirt would report
correct microcode version while still using the old host CPU model
(visible in domain capabilities XML).
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Standardize on putting the _LAST enum value on the second line
of VIR_ENUM_IMPL invocations. Later patches that add string labels
to VIR_ENUM_IMPL will push most of these to the second line anyways,
so this saves some noise.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
If a management application wants to use firmware auto selection
feature it can't currently know if the libvirtd it's talking to
support is or not. Moreover, it doesn't know which values that
are accepted for the @firmware attribute of <os/> when parsing
will allow successful start of the domain later, i.e. if the mgmt
application wants to use 'bios' whether there exists a FW
descriptor in the system that describes bios.
This commit then adds 'firmware' enum to <os/> element in
<domainCapabilities/> XML like this:
<enum name='firmware'>
<value>bios</value>
<value>efi</value>
</enum>
We can see both 'bios' and 'efi' listed which means that there
are descriptors for both found in the system (matched with the
machine type and architecture reported in the domain capabilities
earlier and not shown here).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
virutil.(c|h) is a very gross collection of random code. Remove the enum
handlers from there so we can limit the scope where virtutil.h is used.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Since the STOP event handler can use the pausedReason as sent to
qemuProcessStopCPUs, we no longer need to send duplicate suspended
lifecycle events because we know what caused the stop along with extra
details. This processing allows us to also remove the duplicated state
change from qemuProcessStopCPUs.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
qemuMigrationSrcPerform callers expect it to call virDomainObjEndAPI
in any case so on error paths we miss the virDomainObjEndAPI call.
To fix this let's make qemuMigrationSrcPerform callers responsible
for the virDomainObjEndAPI call.
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
'blockdev-snapshot-sync' is present in QEMU since v0.14.0-rc0 and
'transaction' since v1.1.0 (52e7c241ac766406f05fa)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemu added the 'drive-mirror' command in v1.3.0 (d9b902db3fb71fdc)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemu added the 'block-commit' command in v1.3.0 (ed61fc10e8c8d2)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This was detected by the presence of 'block-stream' which is present in
qemu since v1.1 (db58f9c0605fa151b8c4)
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
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>
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>
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>
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>
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>
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>
Any job which is able to provide statistics that can be queried via
virDomainGetJob{Stats,Info} has to set an appropriate statsType.
Without a proper statsType qemuDomainJobInfoToParams and
qemuDomainJobInfoToInfo have no idea what statistics should be sent to
the API caller.
https://bugzilla.redhat.com/show_bug.cgi?id=1688774
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
snapshot_conf does all the hard work, the qemu driver just has to
accept the new flag.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
virDomainSnapshotDefFormat currently takes two sets of knobs:
an 'unsigned int flags' argument that can currently just be
VIR_DOMAIN_DEF_FORMAT_SECURE, and an 'int internal' argument used as
a bool to determine whether to output an additional element. It
then reuses the 'flags' knob to call into virDomainDefFormatInternal(),
which takes a different set of flags. In fact, prior to commit 0ecd6851
(1.2.12), the 'flags' argument actually took the public
VIR_DOMAIN_XML_SECURE, which was even more confusing. Let's borrow
from the style of that earlier commit, by introducing a function
for translating from the public flags (VIR_DOMAIN_SNAPSHOT_XML_SECURE
was just recently introduced) into a new enum specific to snapshot
formatting, and adjust all callers to use snapshot-specific enum
values when formatting, and where the formatter now uses a new
variable 'domainflags' to make it obvious when we are translating
from snapshot flags back to domain flags. We don't even have to
use the conversion function for drivers that don't accept the
public VIR_DOMAIN_SNAPSHOT_XML_SECURE flag.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Clean up the previous patch which abused switch on virDomainState
while working with a variable containing virDomainSnapshotState, by
converting the two affected switch statements to now use the right
enum.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The existing virDomainSnapshotState is a superset of virDomainState,
adding one more state (disk-snapshot) on top of valid domain states.
But as written, the enum cannot be used for gcc validation that all
enum values are covered in a strongly-typed switch condition, because
the enum does not explicitly include the values it is adding to.
Copy the style used in qemu_blockjob.h of creating new enum names
for every inherited value, and update most clients to use the new
enum names anywhere snapshot state is referenced. The exception is
two switch statements in qemu code, which instead gain a fixme
comment about odd type usage (which will be cleaned up in the next
patch). The rest of the patch is fairly mechanical (I actually did
it by temporarily s/state/xstate/ in snapshot_conf.h to let the
compiler find which spots in the code used the field, did the
obvious search and replace in those functions, then undid the rename).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The current qemu code rejects the combination of the two flags
VIR_DOMAIN_SNAPSHOT_CREATE_LIVE in tandem with
VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, but rather late in the cycle
(after the snapshot was already parsed), and with a rather confusing
message (complaining that live snapshots require external storage,
even if the redefined snapshot already declares external storage).
Hoist the rejection message to occur earlier (before parsing any
XML, which also aids upcoming patches that will implement bulk
redefine), and with a more typical error message about mutually
exclusive flags.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1685151
This reverts commit cefb97fb81.
The stateAutoStart callback will be removed in the next commit.
Therefore move autostarting of domains, networks and storage
pools back into stateInitialize callbacks.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The existing qemu snapshot code has a slight bug: if the domain
is currently pmsuspended, you can't use the _REDEFINE flag even
though the current domain state should have no bearing on being
able to recreate metadata state; and conversely, you can use the
_REDEFINE flag to create snapshot metadata claiming to be
pmsuspended as a bypass to the normal restrictions that you can't
create an original qemu snapshot in that state (the restriction
against pmsuspend is specific to qemu, rather than part of the
driver-agnostic snapshot_conf code).
Fix this by checking the snapshot state (when redefining) instead
of the domain state (which is a subset of snapshot states).
Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Upcoming patches plan to introduce virDomainCheckpointPtr as a new
object for use in incremental backups, along with documentation on
how incremental backups differ from snapshots. But first, we need
to rename any existing mention of a 'system checkpoint' to instead
be a 'full system snapshot', so that we aren't overloading
the term checkpoint.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Right now we're reporting errors in virFileWrapperFdFree(),
but that's hardly the appropriate place to do so, as free
functions are supposed to do nothing more than release
allocated resources.
We want to move that code back into virFileWrapperFdClose(),
but before we can do that we need to make sure the function
is actually called every time we're done processing the
wrapped file. The cleanup path is the obvious candidate.
In a couple of cases we can just move the call, but for the
remaining ones we need to duplicate it instead in order not
to alter the existing behavior. We do, however, make sure
that in all cases a failure to properly close the wrapper
results in the overall operation being reported as failed.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Use refcounting for priv->monConfig instead of asymmetric freeing.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Commit f609cb85 (0.9.5) introduced virDomainSnapshotGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit
28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the snapshot documentation to declare it as invalid.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; esx and vbox don't support flags;
qemu, test, and vz only support VIR_DOMAIN_XML_SECURE), and it is
unlikely that the domain state saved off during a snapshot creation
needs to be migration-friendly (as the snapshot is not the source of
a migration), it is easier to just define an explicit set of supported
flags directly related to the snapshot API rather than trying to
borrow from domain API, and risking confusion if even more domain
flags are added later (in fact, I have an upcoming patch that plans to
add a new flag to virDomainGetXMLDesc that makes no sense for
snapshots).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the reused name).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Commit d2a929d4 (0.9.4) defined virDomainSaveImageGetXMLDesc()'s use
of @flags as a subset of virDomainXMLFlags, documenting that 2 of the
3 flags defined at the time would never be valid. Later, commit
28f8dfdc (1.0.0) introduced a new flag, VIR_DOMAIN_XML_MIGRATABLE, but
did not adjust the save image documentation to declare it as invalid.
Later, commit a67e3872 (3.7.0) blindly copied and pasted the same text
into virDomainManagedSaveGetXMLDesc.
However, since the flag is not accepted as valid by any of the
drivers (remote is just passthrough; and qemu is the only supporting
driver for either API, with support for just VIR_DOMAIN_XML_SECURE),
it is easier to just define an explicit set of supported flags
directly related to the save image API rather than trying to borrow
from live domain API, and risking confusion if even more domain flags
are added later (in fact, I have an upcoming patch that plans to add
a new flag to virDomainGetXMLDesc that makes no sense for saved
images). We may someday decide that saved images need to support the
_MIGRATABLE flag, as it is possible to load a saved image with a
different version of libvirt than the one that created it, but that
can be a separate patch if it is ever needed. Meanwhile, it DOES make
sense to reuse the same flags for SaveImage and for ManagedSave (since
ManagedSave is really just sugar for creating a normal SaveImage in a
location controlled by libvirt instead of by the user).
There is no API or ABI impact (since we purposefully used unsigned int
rather than an enum type in public API, and since the new flag name
carries the same value as the old reused name).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Although VIR_DOMAIN_DEF_FORMAT_INACTIVE and VIR_DOMAIN_XML_INACTIVE
happen to have the same value (1<<1), they come from different enums;
and it is nicer to reason about a 'flags' variable if all uses of
that variable are compared against the same enum type. Messed up in
commit 06f75ff2 (3.8.0).
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Many drivers had a comment that they did not validate the incoming
'flags' to virDomainGetXMLDesc() because they were relying on
virDomainDefFormat() to do it instead. This used to be the case
(at least since 461e0f1a and friends in 0.9.4 added unknown flag
checking in general), but regressed in commit 0ecd6851 (1.2.12),
when all of the drivers were changed to pass 'flags' through the
new helper virDomainDefFormatConvertXMLFlags(). Since this helper
silently ignores unknown flags, we need to implement flag checking
in each driver instead.
Annoyingly, this means that any new flag values added will silently
be ignored when targeting an older libvirt, rather than our usual
practice of loudly diagnosing an unsupported flag. Add comments
in domain_conf.[ch] to remind us to be extra vigilant about the
impact when adding flags (a new flag to add data is safe if the
older server omitting the requested data doesn't break things in
the newer client; a new flag to suppress data rather than enhancing
the existing VIR_DOMAIN_XML_SECURE may form a data leak or even a
security hole).
In the qemu driver, there are multiple callers all funnelling to
qemuDomainDefFormatBufInternal(); many of them already validated
flags (and often only a subset of the full set of possible flags),
but for ease of maintenance, we can also check flags at the common
helper function.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
We dropped support in commit 8e91a40 (November 2015), but some
occurrences still remained, even in live code.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reported-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Now that virStorageSource is a subclass of virObject we can use
virObjectUnref and remove virStorageSourceFree which was a thin wrapper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Since virStorageSource is now a subclass of virObject, we can use
VIR_AUTOUNREF instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Add virStorageSourceNew and refactor places allocating that structure to
use the helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The code for creating external snapshots for an offline domain
called out to qemu-img without escaping commas in the manner
that qemu-img expects. This also fixes a typo in the comment.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Let's make use of the auto __cleanup capabilities cleaning up any
now unnecessary goto paths.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
This flag tells virDomainMigrateSetMaxSpeed and
virDomainMigrateGetMaxSpeed APIs to work on post-copy migration
bandwidth.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Let's make the code flow easier to follow and get rid of the ugly endjob
label inside if branch.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
qemuDomainBlockPivot and qemuDomainBlockJobAbort need the job name for
cancelling or pivoting but were generating it locally instead of
accessing the existing copy in the job data structure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The writing to an image actually starts when the copy job is initiated,
so checking this at the time of the pivot operation is too late.
Move the check to qemuDomainBlockCopyCommon. Note that modern qemu would
have prevented two writers with qcow2 so the slim possibility of a job
started with libvirtd without this patch missing the check is not really
worth worrying about.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
For copy and active commit jobs we record the state of the mirror so
that we can recover. The status XML was not saved in case of
qemuDomainBlockPivot due to an oversight.
Save the XML always when invoking qemuDomainBlockJobAbort even if
the job is not currently tracking any state. This will change later and
also this is not a particularly hot code path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>), and we have a mix of semicolon and
non-semicolon usage through the code. Let's standardize on using
a semicolon for VIR_ENUM_IMPL calls.
Move the verify() statement to the end of the macro and drop
the semicolon, so the compiler will require callers to add a
semicolon.
While we are touching these call sites, standardize on putting
the closing parenth on its own line, as discussed here:
https://www.redhat.com/archives/libvir-list/2019-January/msg00750.html
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>), and we have a mix of semicolon and
non-semicolon usage through the code. Let's standardize on using
a semicolon for VIR_ENUM_DECL calls.
Drop the semicolon from the final statement of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
