Re: [PATCH v2] Improve blockpull man entry

[Pavel Mores]

On Tue, Apr 28, 2020 at 02:23:11PM +, Sebastian Mitterle wrote:
> 1. Fix usage of bandwidth, base arguments.
> 2. Explain valid arguments for `base`.
> 3. Move explanation for `--keep-relative` to end considering it's
>not a very frequent use case.
> 4. Add reference to documentation for relative paths in backing chains.
> 
> Signed-off-by: Sebastian Mitterle 
> ---
>  docs/manpages/virsh.rst | 19 ++-
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index dc404ddfe8..71bd968fab 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -1345,7 +1345,7 @@ blockpull
>  
>  .. code-block::
>  
> -   blockpull domain path [bandwidth] [--bytes] [base]
> +   blockpull domain path [bandwidth [--bytes] [base]]

Shouldn't this be

+   blockpull domain path [bandwidth [--bytes]] [base]

or, perhaps better yet

+   blockpull domain path [--bandwidth  [--bytes]] [base]

>[--wait [--verbose] [--timeout seconds] [--async]]
>[--keep-relative]
>  
> @@ -1356,6 +1356,12 @@ the new backing file and only the intermediate portion 
> of the chain is
>  pulled.  Once all requested data from the backing image chain has been
>  pulled, the disk no longer depends on that portion of the backing chain.
>  
> +*base* can be specified in two ways: either as indexed target name 'name[i]'
> +where 'name' corresponds to the disk target name () and
> +'i' corresponds to the 'index' of the ''; or as the file name
> +of the backing file ().
> +
> +
>  By default, this command returns as soon as possible, and data for
>  the entire disk is pulled in the background; the progress of the
>  operation can be checked with ``blockjob``.  However, if *--wait* is
> @@ -1367,16 +1373,19 @@ is triggered, *--async* will return control to the 
> user as fast as
>  possible, otherwise the command may continue to block a little while
>  longer until the job is done cleaning up.
>  
> -Using the *--keep-relative* flag will keep the backing chain names
> -relative.
> -
>  *path* specifies fully-qualified path of the disk; it corresponds
>  to a unique target name () or source file (  file='name'/>) for one of the disk devices attached to *domain* (see
>  also ``domblklist`` for listing these names).
> +
>  *bandwidth* specifies copying bandwidth limit in MiB/s. For further 
> information
>  on the *bandwidth* argument see the corresponding section for the 
> ``blockjob``
> -command.
> +command. Using *--bytes* flag indicates the value in *bandwidth* is given in
> +bytes.
> +
> +Using the *--keep-relative* flag will keep the backing chain names
> +relative (details on `https://www.libvirt.org/kbase/backing_chains.html
> +`__).
>  
>  
>  blockresize
> -- 
> 2.25.2
> 

[libvirt PATCH v2 07/10] qemu: block: add function to launch all prepared blockcommits

[Pavel Mores]

This is the second phase of snapshot deletion.  We have all information
necessary to delete the snapshot by running blockcommits and we haven't
detected any problems that would make the deletion unsafe.

Now we just launch the blockcommits in parallel.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dc1176bd9c..35b7fb69d5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -154,6 +154,16 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t 
fallback_gid,
 
 static virQEMUDriverPtr qemu_driver;
 
+static int
+qemuDomainBlockCommitCommon(virDomainObjPtr vm,
+virQEMUDriverPtr driver,
+virDomainDiskDefPtr disk,
+virStorageSourcePtr baseSource,
+virStorageSourcePtr topSource,
+virStorageSourcePtr topParentSource,
+unsigned long bandwidth,
+unsigned int flags);
+
 static int
 qemuDomainBlockCommitImpl(virDomainObjPtr vm,
   virQEMUDriverPtr driver,
@@ -16906,6 +16916,31 @@ 
qemuDomainSnapshotDeleteExternalGetJobDescriptors(virDomainObjPtr vm,
 }
 
 
+static int
+qemuDomainSnapshotDeleteExternalLaunchJobs(virDomainObjPtr vm,
+   virQEMUDriverPtr driver,
+   const virBlockCommitDesc 
*blockCommitDescs,
+   int numDescs)
+{
+size_t i;
+
+for (i = 0; i < numDescs; i++) {
+virDomainDiskDefPtr disk = blockCommitDescs[i].disk;
+virStorageSourcePtr baseSource = blockCommitDescs[i].baseSource;
+virStorageSourcePtr topSource = blockCommitDescs[i].topSource;
+virStorageSourcePtr topParentSource = 
blockCommitDescs[i].topParentSource;
+int blockCommitFlags = blockCommitDescs[i].blockCommitFlags;
+
+if (qemuDomainBlockCommitCommon(vm, driver, disk, baseSource,
+topSource, topParentSource,
+0, blockCommitFlags) < 0)
+return -1;
+}
+
+return 0;
+}
+
+
 static int
 qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
  unsigned int flags)
-- 
2.24.1

[libvirt PATCH v2 00/10] qemu: block: basic implementation of deletion of external snapshots

[Pavel Mores]

Deleting external snapshots has been unimplemented so far.  This series aims
to enable limited functionality in that direction, handling just the common
cases for now.  The intention is to subsequently build upon this to eventually
cover the more complex/exotic cases as well.

It works by blockcommitting the snapshot to be deleted into its parent.
Handles both deleting children as well and deleting children only (by
committing the children into the snapshot referred to by snapshot-delete
argument).  If the necessary block commit operations turns out to be active
(the snapshot referred to by snapshot-delete argument is current, or
deleting children is requested) pivot is performed as well.

This implemetation is limited to the straightforward case which assumes no
branching snapshot chains below the snapshot to be deleted, and the current
snapshot has to be the leaf of the (linear) chain starting at the snapshot
to be deleted.  These requirements are checked and enforced at the top of
qemuDomainSnapshotDeleteExternal().  They might even be too restrictive for
the case where deletion of children is not requested as in that case,
requiring that the parent of the snapshot to be deleted not be a branching
point in snapshot tree should be enough.

The above limitations do not appear too severe under the current state of
matters where a major source of branching snapshot structures,
snapshot-revert, is not even implemented for external snapshots yet.  The
only other known cause of branching in external snapshots thus seems to be
if a user creates branching by manually creating snapshot images and
metadata and applies them using snapshot-create --redefine.

Also, the snapshot images involved basically have to be files.  This does
not seem overly limiting either, at least for the time being, because it's
a limitation shared with the underlying blockcommit code.  Just as the
existing code, the new code ultimately relies on
virStorageFileChainLookup() as its image comparison and look-up engine and
is affected by its limitaions.

At any rate, this work should be understood as just a first step to a full
support of deleting external snapshots.

The first 5 commits of this series are just refactors used by the last 5
commits which actually implement snapshot deletion.  However, the
qemuDomainBlockCommit() refactor (the initial 2 commits of this series) was
affected by a rather hairy merge on rebasing to the current master and
although I tried to do my best to get the merge right I still feel a
careful review is in order for these two commits.

Pavel Mores (10):
  qemu: block: factor implementation out of qemuDomainBlockCommit()
  qemu: block: refactor blockcommit so that it's callable with storage
sources
  qemu: block: factor implementation out of qemuDomainBlockJobAbort()
  conf: rename virDomainMomentFindLeaf() to
virDomainMomentObjListFindLeaf()
  conf: add virDomainMomentFindLeaf() which operates on
virDomainMomentObjPtr
  qemu: block: add function to collect blockcommit params and verify
them
  qemu: block: add function to launch all prepared blockcommits
  qemu: block: add function to wait for blockcommits and collect results
  qemu: block: add external snapshot-delete top-level algorithm
  qemu: block: add actual invocation of external snapshot-delete

 src/conf/virdomaincheckpointobjlist.c |   2 +-
 src/conf/virdomainmomentobjlist.c |  27 +-
 src/conf/virdomainmomentobjlist.h |   3 +-
 src/libvirt_private.syms  |   1 +
 src/qemu/qemu_driver.c| 552 ++
 5 files changed, 485 insertions(+), 100 deletions(-)

-- 
2.24.1

[libvirt PATCH v2 01/10] qemu: block: factor implementation out of qemuDomainBlockCommit()

[Pavel Mores]

We'll need blockcommit to be callable from within the QEMU driver where we
have no virDomain instance available, just virDomainObj.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 78 --
 1 file changed, 52 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1c7c87128d..b642b24fa2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -154,6 +154,15 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t 
fallback_gid,
 
 static virQEMUDriverPtr qemu_driver;
 
+static int
+qemuDomainBlockCommitImpl(virDomainObjPtr vm,
+  virQEMUDriverPtr driver,
+  const char *path,
+  const char *base,
+  const char *top,
+  unsigned long bandwidth,
+  unsigned int flags);
+
 /* Looks up the domain object from snapshot and unlocks the
  * driver. The returned domain object is locked and ref'd and the
  * caller must call virDomainObjEndAPI() on it. */
@@ -18431,18 +18440,16 @@ qemuDomainBlockPull(virDomainPtr dom, const char 
*path, unsigned long bandwidth,
 return qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags);
 }
 
-
 static int
-qemuDomainBlockCommit(virDomainPtr dom,
-  const char *path,
-  const char *base,
-  const char *top,
-  unsigned long bandwidth,
-  unsigned int flags)
+qemuDomainBlockCommitImpl(virDomainObjPtr vm,
+  virQEMUDriverPtr driver,
+  const char *path,
+  const char *base,
+  const char *top,
+  unsigned long bandwidth,
+  unsigned int flags)
 {
-virQEMUDriverPtr driver = dom->conn->privateData;
-qemuDomainObjPrivatePtr priv;
-virDomainObjPtr vm = NULL;
+qemuDomainObjPrivatePtr priv = vm->privateData;
 const char *device = NULL;
 const char *jobname = NULL;
 int ret = -1;
@@ -18466,22 +18473,6 @@ qemuDomainBlockCommit(virDomainPtr dom,
 g_autoptr(virJSONValue) bitmapDisableActions = NULL;
 VIR_AUTOSTRINGLIST bitmapDisableList = NULL;
 
-virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
-  VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
-  VIR_DOMAIN_BLOCK_COMMIT_RELATIVE |
-  VIR_DOMAIN_BLOCK_COMMIT_DELETE |
-  VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1);
-
-if (!(vm = qemuDomainObjFromDomain(dom)))
-goto cleanup;
-priv = vm->privateData;
-
-if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0)
-goto cleanup;
-
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
-goto cleanup;
-
 if (virDomainObjCheckActive(vm) < 0)
 goto endjob;
 
@@ -18730,6 +18721,40 @@ qemuDomainBlockCommit(virDomainPtr dom,
 virErrorRestore(_err);
 }
 qemuBlockJobStartupFinalize(vm, job);
+
+return ret;
+}
+
+
+static int
+qemuDomainBlockCommit(virDomainPtr dom,
+  const char *path,
+  const char *base,
+  const char *top,
+  unsigned long bandwidth,
+  unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+int ret = -1;
+
+virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
+  VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
+  VIR_DOMAIN_BLOCK_COMMIT_RELATIVE |
+  VIR_DOMAIN_BLOCK_COMMIT_DELETE |
+  VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1);
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+ret = qemuDomainBlockCommitImpl(vm, driver, path, base, top, bandwidth, 
flags);
+
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
@@ -18737,6 +18762,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
 return ret;
 }
 
+
 static int
 qemuDomainOpenGraphics(virDomainPtr dom,
unsigned int idx,
-- 
2.24.1

[libvirt PATCH v2 08/10] qemu: block: add function to wait for blockcommits and collect results

[Pavel Mores]

This is the third phase of snapshot deletion.  Blockcommits to delete the
snapshot have been launched and now we can wait for them to finish, check
results and report errors if any.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 59 ++
 1 file changed, 59 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 35b7fb69d5..a2629e9002 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16941,6 +16941,65 @@ 
qemuDomainSnapshotDeleteExternalLaunchJobs(virDomainObjPtr vm,
 }
 
 
+static int
+qemuDomainSnapshotDeleteExternalWaitForJobs(virDomainObjPtr vm,
+virQEMUDriverPtr driver,
+const virBlockCommitDesc 
*blockCommitDescs,
+int numDescs)
+{
+size_t i;
+
+for (i = 0; i < numDescs; i++) {
+virDomainDiskDefPtr disk = blockCommitDescs[i].disk;
+bool isActive = blockCommitDescs[i].isActive;
+
+/* wait for the blockcommit job to finish (in particular, reach
+ * one of the finished QEMU_BLOCKJOB_STATE_* states)... */
+g_autoptr(qemuBlockJobData) job = NULL;
+
+if (!(job = qemuBlockJobDiskGetJob(disk))) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+_("disk %s does not have an active block job"), 
disk->dst);
+return -1;
+}
+
+qemuBlockJobSyncBegin(job);
+qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
+while (job->state != QEMU_BLOCKJOB_STATE_READY &&
+   job->state != QEMU_BLOCKJOB_STATE_FAILED &&
+   job->state != QEMU_BLOCKJOB_STATE_CANCELLED &&
+   job->state != QEMU_BLOCKJOB_STATE_COMPLETED) {
+
+if (virDomainObjWait(vm) < 0)
+return -1;
+qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
+}
+qemuBlockJobSyncEnd(vm, job, QEMU_ASYNC_JOB_NONE);
+
+if ((isActive && job->state != QEMU_BLOCKJOB_STATE_READY) ||
+(!isActive && job->state != QEMU_BLOCKJOB_STATE_COMPLETED)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+_("blockcomit job failed for disk %s"), disk->dst);
+/* TODO Apr 30, 2020: how to handle this?  Bailing out doesn't
+ * seem an obvious option in this case as all blockjobs are now
+ * created and running - if any of them are to fail they will,
+ * regardless of whether we break here.  It might make more
+ * sense to continue and at least report all errors. */
+/*return -1;*/
+}
+
+/* ... and pivot if necessary */
+if (isActive) {
+if (qemuDomainBlockJobAbortImpl(driver, vm, disk->dst,
+VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) 
< 0)
+return -1;
+}
+}
+
+return 0;
+}
+
+
 static int
 qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
  unsigned int flags)
-- 
2.24.1

[libvirt PATCH v2 02/10] qemu: block: refactor blockcommit so that it's callable with storage sources

[Pavel Mores]

So far, only paths could be used to specify blockcommit's base and top
images.  However, this is not general enough as paths have limitations
(most notably they can only work for file-based storage sources).

This commit preserves the path-based interface but factors out the core of
blockcommit implementation into a separate function that takes its base and
top as virStorageSources.  The path-based interface basically just converts
the paths into virStorageSource just as it was done before and calls the
virStorageSource-based implementation core.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 108 +
 1 file changed, 65 insertions(+), 43 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b642b24fa2..09300c1e90 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18441,25 +18441,27 @@ qemuDomainBlockPull(virDomainPtr dom, const char 
*path, unsigned long bandwidth,
 }
 
 static int
-qemuDomainBlockCommitImpl(virDomainObjPtr vm,
-  virQEMUDriverPtr driver,
-  const char *path,
-  const char *base,
-  const char *top,
-  unsigned long bandwidth,
-  unsigned int flags)
+qemuDomainBlockCommitCommon(virDomainObjPtr vm,
+virQEMUDriverPtr driver,
+virDomainDiskDefPtr disk,
+virStorageSourcePtr baseSource,
+virStorageSourcePtr topSource,
+virStorageSourcePtr topParentSource,
+unsigned long bandwidth,
+unsigned int flags)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 const char *device = NULL;
 const char *jobname = NULL;
 int ret = -1;
-virDomainDiskDefPtr disk = NULL;
-virStorageSourcePtr topSource;
-unsigned int topIndex = 0;
-virStorageSourcePtr baseSource = NULL;
-unsigned int baseIndex = 0;
-virStorageSourcePtr top_parent = NULL;
 bool clean_access = false;
+/* TODO the following 2 are just for error reporting. Originally these
+ * could have been either paths or indexed identifiers (like 'vda[1]').
+ * Now the error reporting will always use paths instead of what the
+ * user originally specified.  Find out if this is fine, find a solution
+ * if it isn't. */
+const char *path = disk->src->path;
+const char *base = baseSource->path;
 g_autofree char *topPath = NULL;
 g_autofree char *basePath = NULL;
 g_autofree char *backingPath = NULL;
@@ -18473,9 +18475,6 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm,
 g_autoptr(virJSONValue) bitmapDisableActions = NULL;
 VIR_AUTOSTRINGLIST bitmapDisableList = NULL;
 
-if (virDomainObjCheckActive(vm) < 0)
-goto endjob;
-
 blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
 
 /* Convert bandwidth MiB to bytes, if necessary */
@@ -18489,9 +18488,6 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm,
 speed <<= 20;
 }
 
-if (!(disk = qemuDomainDiskByName(vm->def, path)))
-goto endjob;
-
 if (virStorageSourceIsEmpty(disk->src)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("disk %s has no source file to be committed"),
@@ -18511,14 +18507,6 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm,
 goto endjob;
 }
 
-if (!top || STREQ(top, disk->dst))
-topSource = disk->src;
-else if (virStorageFileParseChainIndex(disk->dst, top, ) < 0 ||
- !(topSource = virStorageFileChainLookup(disk->src, NULL,
- top, topIndex,
- _parent)))
-goto endjob;
-
 if (topSource == disk->src) {
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ACTIVE_COMMIT)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -18546,13 +18534,6 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm,
 goto endjob;
 }
 
-if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW))
-baseSource = topSource->backingStore;
-else if (virStorageFileParseChainIndex(disk->dst, base, ) < 0 ||
- !(baseSource = virStorageFileChainLookup(disk->src, topSource,
-  base, baseIndex, NULL)))
-goto endjob;
-
 if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) &&
 baseSource != topSource->backingStore) {
 virReportError(VIR_ERR_INVALID_ARG,
@@ -18580,8 +18561,8 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm,
 goto endjob;
 }
 
-if (blockdev && top_parent &&
-qemuBlockUpdateRelativeBack

[libvirt PATCH v2 06/10] qemu: block: add function to collect blockcommit params and verify them

[Pavel Mores]

Snapshot deletion is implemented as a three-phase process - first we
collect specifications of blockcommits used to perform the deletion and
run sanity checks to verify that the whole operation is reasonably safe,
then we launch the blockcommits and finally we wait for them to finish and
check the results.

This commit implements the first phase.  All information necessary to
launch and finish a blockcommit job per disk included in the snapshot is
collected in a blockcommit descriptor structure and checked to verify the
job makes sense and has a reasonable chance to succeed.  Broadly speaking,
the checks aim to ensure that the disks in the current running VM are the
same as they were when the snapshot was created.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 154 +
 1 file changed, 154 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6ffd53503b..dc1176bd9c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16752,6 +16752,160 @@ qemuDomainMomentReparentChildren(void *payload,
 }
 
 
+/* Blockcommit operation descriptor.  Holds data required to launch and
+ * conclude an individual blockcommit job.  Generic parameters
+ * (virDomainObjPtr, virQEMUDriverPtr) are not stored as they are assumed to be
+ * available at the point of blockcommit invocation. */
+
+typedef struct {
+virDomainDiskDefPtr disk;
+virStorageSourcePtr baseSource;
+virStorageSourcePtr topSource;
+virStorageSourcePtr topParentSource;
+int blockCommitFlags;
+bool isActive;  /* used to find out if pivot is needed to finish the job */
+} virBlockCommitDesc;
+
+/* Transforms a snapshot into an array of descriptors of blockcommit jobs
+ * required to delete the snapshot, one descriptor per affected disk.  Also
+ * runs sanity checks for each affected disk and each individual job to make
+ * sure the deletion is safe to perform.  Returns NULL if safety cannot be
+ * guaranteed.
+ *
+ * (A snapshot is considered safe to delete if, loosely speaking, we can be
+ * reasonably sure that all disks it touches are still those that were there
+ * when the snapshot was created.  For instance, a different disk might be
+ * attached to the VM under the same target name, or a disk included in the
+ * snapshot might be reattached to the VM under a different target name.  If
+ * any such thing happens between the times of snapshot creation and deletion,
+ * that shapshot would not be considered safe to delete.) */
+
+static virBlockCommitDesc *
+qemuDomainSnapshotDeleteExternalGetJobDescriptors(virDomainObjPtr vm,
+virDomainMomentObjPtr snap,
+unsigned int flags)
+{
+size_t i;
+bool isActive;
+virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap);
+virDomainMomentObjPtr parent = snap->parent;
+virDomainSnapshotDefPtr parentdef = virDomainSnapshotObjGetDef(parent);
+g_autofree virBlockCommitDesc *blockCommitDescs = 
g_new(virBlockCommitDesc, snapdef->ndisks);
+int blockCommitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE;
+
+if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
+ VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) {
+isActive = true;
+} else {
+isActive = snap == virDomainSnapshotGetCurrent(vm->snapshots);
+}
+
+if (isActive)
+blockCommitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE;
+
+for (i = 0; i < snapdef->ndisks; i++) {
+virDomainSnapshotDiskDefPtr snapDisk = &(snapdef->disks[i]);
+const char *diskName = snapDisk->name;
+virStorageSourcePtr baseSource = NULL;
+virStorageSourcePtr topSource = NULL;
+virStorageSourcePtr topParentSource = NULL;
+virDomainDiskDefPtr domDiskNow;
+virDomainDiskDefPtr domDiskThen = 
virDomainDiskByTarget(snapdef->parent.dom, diskName);
+virStorageSourcePtr snapSource = NULL;
+virStorageSourcePtr snapParentSource = NULL;
+unsigned int snapIndex;
+
+if (!(domDiskNow = qemuDomainDiskByName(vm->def, diskName))) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("can't delete %s, disk '%s' not found in running 
VM"),
+   snapdef->parent.name, diskName);
+return NULL;
+}
+if (snapDisk->src->type != VIR_STORAGE_TYPE_FILE ||
+domDiskNow->src->type != VIR_STORAGE_TYPE_FILE) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("can't delete %s, only storage type 'file' is 
supported"),
+   snapdef->parent.name);
+return NULL;
+}
+
+/* TODO Apr 24, 2020: this is not great long-term as it still looks up
+ * essentially just by path (or target[index]), far from a full
+ * comparison ope

[libvirt PATCH v2 05/10] conf: add virDomainMomentFindLeaf() which operates on virDomainMomentObjPtr

[Pavel Mores]

The new function operates on virDomainMomentObjPtr which is indicated by
its name.  To avoid code duplication, virDomainMomentObjListFindLeaf()
was reimplemented in terms of the new function.

Signed-off-by: Pavel Mores 
---
 src/conf/virdomainmomentobjlist.c | 25 +++--
 src/conf/virdomainmomentobjlist.h |  1 +
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/conf/virdomainmomentobjlist.c 
b/src/conf/virdomainmomentobjlist.c
index 41e887ba18..c57d9cc787 100644
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
@@ -207,6 +207,20 @@ virDomainMomentMoveChildren(virDomainMomentObjPtr from,
 }
 
 
+/* If there is exactly one leaf node, return that node. */
+virDomainMomentObjPtr
+virDomainMomentFindLeaf(virDomainMomentObjPtr moment)
+{
+if (moment->nchildren != 1)
+return NULL;
+while (moment->nchildren == 1)
+moment = moment->first_child;
+if (moment->nchildren == 0)
+return moment;
+return NULL;
+}
+
+
 static virDomainMomentObjPtr
 virDomainMomentObjNew(void)
 {
@@ -586,17 +600,8 @@ virDomainMomentCheckCycles(virDomainMomentObjListPtr list,
 return 0;
 }
 
-/* If there is exactly one leaf node, return that node. */
 virDomainMomentObjPtr
 virDomainMomentObjListFindLeaf(virDomainMomentObjListPtr list)
 {
-virDomainMomentObjPtr moment = >metaroot;
-
-if (moment->nchildren != 1)
-return NULL;
-while (moment->nchildren == 1)
-moment = moment->first_child;
-if (moment->nchildren == 0)
-return moment;
-return NULL;
+return virDomainMomentFindLeaf(>metaroot);
 }
diff --git a/src/conf/virdomainmomentobjlist.h 
b/src/conf/virdomainmomentobjlist.h
index cab51edbc4..9c2d01ff8f 100644
--- a/src/conf/virdomainmomentobjlist.h
+++ b/src/conf/virdomainmomentobjlist.h
@@ -62,6 +62,7 @@ void virDomainMomentMoveChildren(virDomainMomentObjPtr from,
  virDomainMomentObjPtr to);
 void virDomainMomentLinkParent(virDomainMomentObjListPtr moments,
virDomainMomentObjPtr moment);
+virDomainMomentObjPtr virDomainMomentFindLeaf(virDomainMomentObjPtr moment);
 
 virDomainMomentObjListPtr virDomainMomentObjListNew(void);
 void virDomainMomentObjListFree(virDomainMomentObjListPtr moments);
-- 
2.24.1

[libvirt PATCH v2 04/10] conf: rename virDomainMomentFindLeaf() to virDomainMomentObjListFindLeaf()

[Pavel Mores]

We'll need the original name for a function that finds leaf for a single
virDomainMomentObj.  The new name properly advertises that the function
actually works on a virDomainMomentObjList, as some (but not all) of
existing functions operating on lists do.

Signed-off-by: Pavel Mores 
---
 src/conf/virdomaincheckpointobjlist.c | 2 +-
 src/conf/virdomainmomentobjlist.c | 2 +-
 src/conf/virdomainmomentobjlist.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/conf/virdomaincheckpointobjlist.c 
b/src/conf/virdomaincheckpointobjlist.c
index a4942ea706..2df1c426a2 100644
--- a/src/conf/virdomaincheckpointobjlist.c
+++ b/src/conf/virdomaincheckpointobjlist.c
@@ -186,7 +186,7 @@ 
virDomainCheckpointUpdateRelations(virDomainCheckpointObjListPtr checkpoints,
 int ret = virDomainMomentUpdateRelations(checkpoints->base);
 
 if (ret == 0)
-*leaf = virDomainMomentFindLeaf(checkpoints->base);
+*leaf = virDomainMomentObjListFindLeaf(checkpoints->base);
 return ret;
 }
 
diff --git a/src/conf/virdomainmomentobjlist.c 
b/src/conf/virdomainmomentobjlist.c
index 18dbd434fb..41e887ba18 100644
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
@@ -588,7 +588,7 @@ virDomainMomentCheckCycles(virDomainMomentObjListPtr list,
 
 /* If there is exactly one leaf node, return that node. */
 virDomainMomentObjPtr
-virDomainMomentFindLeaf(virDomainMomentObjListPtr list)
+virDomainMomentObjListFindLeaf(virDomainMomentObjListPtr list)
 {
 virDomainMomentObjPtr moment = >metaroot;
 
diff --git a/src/conf/virdomainmomentobjlist.h 
b/src/conf/virdomainmomentobjlist.h
index 75198909ba..cab51edbc4 100644
--- a/src/conf/virdomainmomentobjlist.h
+++ b/src/conf/virdomainmomentobjlist.h
@@ -121,4 +121,4 @@ int 
virDomainMomentUpdateRelations(virDomainMomentObjListPtr moments);
 int virDomainMomentCheckCycles(virDomainMomentObjListPtr list,
virDomainMomentDefPtr def,
const char *domname);
-virDomainMomentObjPtr virDomainMomentFindLeaf(virDomainMomentObjListPtr list);
+virDomainMomentObjPtr virDomainMomentObjListFindLeaf(virDomainMomentObjListPtr 
list);
-- 
2.24.1

[libvirt PATCH v2 03/10] qemu: block: factor implementation out of qemuDomainBlockJobAbort()

[Pavel Mores]

Just as with the qemuDomainBlockCommit() refactor in the commit before the
previous one, the motivation is primarily to make blockjob abortion
callable from within the QEMU driver where only virDomainObj instance is
available but no virDomain.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 62 --
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 09300c1e90..6ffd53503b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -163,6 +163,12 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm,
   unsigned long bandwidth,
   unsigned int flags);
 
+static int
+qemuDomainBlockJobAbortImpl(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+const char *path,
+unsigned int flags);
+
 /* Looks up the domain object from snapshot and unlocks the
  * driver. The returned domain object is locked and ref'd and the
  * caller must call virDomainObjEndAPI() on it. */
@@ -17541,45 +17547,31 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm,
 return ret;
 }
 
-
 static int
-qemuDomainBlockJobAbort(virDomainPtr dom,
-const char *path,
-unsigned int flags)
+qemuDomainBlockJobAbortImpl(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+const char *path,
+unsigned int flags)
 {
-virQEMUDriverPtr driver = dom->conn->privateData;
 virDomainDiskDefPtr disk = NULL;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT);
 bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC);
 g_autoptr(qemuBlockJobData) job = NULL;
-virDomainObjPtr vm;
 qemuDomainObjPrivatePtr priv = NULL;
 bool blockdev = false;
 int ret = -1;
 
-virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
-  VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
-
-if (!(vm = qemuDomainObjFromDomain(dom)))
-return -1;
-
-if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0)
-goto cleanup;
-
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
-goto cleanup;
-
 if (virDomainObjCheckActive(vm) < 0)
-goto endjob;
+return -1;
 
 if (!(disk = qemuDomainDiskByName(vm->def, path)))
-goto endjob;
+return -1;
 
 if (!(job = qemuBlockJobDiskGetJob(disk))) {
 virReportError(VIR_ERR_INVALID_ARG,
_("disk %s does not have an active block job"), 
disk->dst);
-goto endjob;
+return -1;
 }
 
 priv = vm->privateData;
@@ -17650,6 +17642,34 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
  endjob:
 if (job && !async)
 qemuBlockJobSyncEnd(vm, job, QEMU_ASYNC_JOB_NONE);
+
+return ret;
+}
+
+
+static int
+qemuDomainBlockJobAbort(virDomainPtr dom,
+const char *path,
+unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+int ret = -1;
+
+virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
+  VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+return -1;
+
+if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+ret = qemuDomainBlockJobAbortImpl(driver, vm, path, flags);
+
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
-- 
2.24.1

[libvirt PATCH v2 10/10] qemu: block: add actual invocation of external snapshot-delete

[Pavel Mores]

Plug in the external snapshot deletion framework built in previous commits.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 57e81e3720..34c0e27eb9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17106,10 +17106,8 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
  qemuDomainSnapshotCountExternal,
  );
 if (external) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("deletion of %d external disk snapshots not "
- "supported yet"), external);
-goto endjob;
+if (qemuDomainSnapshotDeleteExternal(vm, driver, snap, flags) < 0)
+goto endjob;
 }
 }
 
-- 
2.24.1

[libvirt PATCH v2 09/10] qemu: block: add external snapshot-delete top-level algorithm

[Pavel Mores]

This uses the helpers introduced in previous three commits and assembles
the top-level snapshot deletion algorithm in their terms.

Note that the third phase - waiting for blockcommits to finish - is
conceptually optional and a flag/parameter might be exposed to the user in
the future to skip the third phase.  This would make the whole operation
asynchronous and let the user deal with concluding the blockcommits
manually, using the generic blockjob tools.

Signed-off-by: Pavel Mores 
---
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_driver.c   | 64 
 2 files changed, 65 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 935ef7303b..f68eb500ec 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1049,6 +1049,7 @@ virDomainListCheckpoints;
 # conf/virdomainmomentobjlist.h
 virDomainMomentDropChildren;
 virDomainMomentDropParent;
+virDomainMomentFindLeaf;
 virDomainMomentForEachChild;
 virDomainMomentForEachDescendant;
 virDomainMomentMoveChildren;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a2629e9002..57e81e3720 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17000,6 +17000,70 @@ 
qemuDomainSnapshotDeleteExternalWaitForJobs(virDomainObjPtr vm,
 }
 
 
+static int
+qemuDomainSnapshotDeleteExternal(virDomainObjPtr vm,
+ virQEMUDriverPtr driver,
+ virDomainMomentObjPtr snap,
+ unsigned int flags)
+{
+/* TODO Apr 29, 2020: ultimately, use 'flags' to set this.  Until that
+ * is supported, just run always synchronously. */
+bool async = false;
+virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap);
+virDomainMomentObjPtr leaf = snap->nchildren ? 
virDomainMomentFindLeaf(snap) : snap;
+virDomainMomentObjPtr parent = snap->parent;
+g_autofree virBlockCommitDesc *blockCommitDescs = NULL;
+int numBlockCommits = snapdef->ndisks;
+
+/* This function only works if the chain below 'snap' is linear.  If
+ * there's no unique leaf it means the chain of 'snap's children
+ * branches at some point.  Also, if there *is* a leaf but it's not
+ * the current snapshot, bail out as well. */
+if (leaf == NULL) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("can't delete '%s', snapshot chain branches"),
+   snapdef->parent.name);
+return -1;
+}
+if (leaf != virDomainSnapshotGetCurrent(vm->snapshots)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("can't delete '%s', leaf snapshot is not current"),
+   snapdef->parent.name);
+return -1;
+}
+if (parent->nchildren > 1) {
+/* TODO 'snap's parent has multiple children, meaning it's a
+ * branching point in snapshot tree.  This means we can't
+ * delete 'snap' by commiting into its parent as doing so would
+ * corrupt the other branches rooted in the parent.  We might
+ * still be able to delete 'snap' though by pulling into its
+ * child/children. */
+
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("can't delete %s, its parent has multiple children"),
+   snapdef->parent.name);
+return -1;
+}
+
+if (!virDomainObjIsActive(vm))
+return -1;
+
+blockCommitDescs = qemuDomainSnapshotDeleteExternalGetJobDescriptors(vm, 
snap, flags);
+if (blockCommitDescs == NULL)
+return -1;
+
+if (qemuDomainSnapshotDeleteExternalLaunchJobs(vm, driver, 
blockCommitDescs, numBlockCommits) < 0)
+return -1;
+
+if (!async) {
+if (qemuDomainSnapshotDeleteExternalWaitForJobs(vm, driver, 
blockCommitDescs, numBlockCommits) < 0)
+return -1;
+}
+
+return 0;
+}
+
+
 static int
 qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
  unsigned int flags)
-- 
2.24.1

Re: [PATCH v2 0/5] qemu: spport AIO interface io_uring

[Pavel Mores]

On Tue, Apr 21, 2020 at 08:19:33PM +0800, Han Han wrote:
> Changes from v1:
> - Separate the news XML patch from the docs patch
> - More details in the descrition of news XML
> - Change bus type of unit tests XML from ide to virtio
> - Fix missing spaces in docs patch
> - Reorder the sequence of patches
> 
> git repo: https://github.com/qiankehan/libvirt/tree/io_uring-v2
> v1: https://www.redhat.com/archives/libvir-list/2020-April/msg00883.html
> 
> Han Han (5):
>   qemu_capabilities: Introduce QEMU_CAPS_AIO_IO_URING
>   qemu: Implement the aio mode io_uring
>   docs: Docs and rng schemas for io_uring
>   tests: Tests for io mode io_uring
>   news: qemu: support async IO mode 'io_uring'

This series does pretty much what's requested by

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

am I right?

Thanks,

pvl

Re: [PATCH] qemu: Move interlocking of blockjobs and checkpoints after liveness check

[Pavel Mores]

On Thu, Apr 23, 2020 at 04:13:43PM +0200, Peter Krempa wrote:
> qemuDomainSupportsCheckpointsBlockjobs checks if the
> QEMU_CAPS_INCREMENTAL_BACKUP capability is supported to do the
> interlocking. Capabilities are not present when the VM isn't running
> though which would create false errors.
> 
> Move the checks after the liveness check in block job implementations.
> 
> Signed-off-by: Peter Krempa 

Reviewed-by: Pavel Mores 

> ---
>  src/qemu/qemu_driver.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 00d276061e..5ecf12deef 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17439,6 +17439,9 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm,
>  if (virDomainObjCheckActive(vm) < 0)
>  goto endjob;
> 
> +if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0)
> +goto endjob;
> +
>  if (!(disk = qemuDomainDiskByName(vm->def, path)))
>  goto endjob;
> 
> @@ -17994,6 +17997,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  if (virDomainObjCheckActive(vm) < 0)
>  goto endjob;
> 
> +if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0)
> +goto endjob;
> +
>  if (!(disk = qemuDomainDiskByName(vm->def, path)))
>  goto endjob;
> 
> @@ -18278,9 +18284,6 @@ qemuDomainBlockRebase(virDomainPtr dom, const char 
> *path, const char *base,
>  if (virDomainBlockRebaseEnsureACL(dom->conn, vm->def) < 0)
>  goto cleanup;
> 
> -if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0)
> -goto cleanup;
> -
>  /* For normal rebase (enhanced blockpull), the common code handles
>   * everything, including vm cleanup. */
>  if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY))
> @@ -18364,9 +18367,6 @@ qemuDomainBlockCopy(virDomainPtr dom, const char 
> *disk, const char *destxml,
>  if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0)
>  goto cleanup;
> 
> -if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0)
> -goto cleanup;
> -
>  for (i = 0; i < nparams; i++) {
>  virTypedParameterPtr param = [i];
> 
> @@ -18429,11 +18429,6 @@ qemuDomainBlockPull(virDomainPtr dom, const char 
> *path, unsigned long bandwidth,
>  return -1;
>  }
> 
> -if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) {
> -virDomainObjEndAPI();
> -return -1;
> -}
> -
>  /* qemuDomainBlockPullCommon consumes the reference on @vm */
>  return qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags);
>  }
> -- 
> 2.26.0
> 

Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases

[Pavel Mores]

On Wed, Apr 22, 2020 at 03:49:57PM +0200, Peter Krempa wrote:
> On Thu, Apr 16, 2020 at 17:07:35 +0200, Pavel Mores wrote:
> > On Fri, Apr 03, 2020 at 05:56:59PM +0200, Pavel Mores wrote:
> > > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote:
> > > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote:
> > > > > +}
> > > > > +
> > > > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)
> > > > > +topPath = disk->src->path;
> > > > > +else
> > > > > +topPath = snapdisk->src->path;
> > > > 
> > > > You must not use paths for doing this lookup. Paths at best work for
> > > > local files only and this would make the code not future proof.
> > > > 
> > > > Also you want to verify that:
> > > > - images you want to merge are actually in the backing chain
> > > > - the backing chain looks as snapshots describe it (e.g you unplug vda
> > > >   and plug a different chain back)
> > > 
> > > Let me check with you how exactly to perform this verification.
> > > 
> > > To retrieve the names of the disks included in a snapshot, I can use its
> > > virDomainSnapshotDef which contains an array of disks
> > > (virDomainSnapshotDiskDef's) whose 'name' member can be used to identify 
> > > disks.
> > > 
> > > To get the state of the chain at moment the snapshot was created, I can 
> > > use the
> > > 'src' member and walk its 'backingStore' list to examine the whole chain.
> > > 
> > > To get the currently running configuration, I assume I can use the names 
> > > of the
> > > relevant disks (obtained from the snapshot as mentioned above) to get a
> > > virDomainDiskDefPtr for each of them, whose 'src' member points to a
> > > virStorageSource that represents the topmost image in the disk's chain.  
> > > From
> > > there, I can walk the 'backingStore' list to get the other images in the 
> > > chain
> > > all the way to the base image.
> > > 
> > > The verification succeeds if the disk names and their chains in the 
> > > snapshot
> > > and the running config correspond.  Is the above correct?
> > > 
> > > If so, I'm still not certain how to compare two virStorageSource's.  How 
> > > is
> > > identity of two different virStorageSource instances is established?
> > 
> > After having a closer look into this I'm unfortunately even less clear as to
> > how to perform the checks mentioned in the review.
> 
> Well unfortunately developing this is the main part why deleting
> snapshots is complicated.
> 
> > In addition to the problem of establishing virStorageSource identity, a 
> > similar
> > problem seems to come up with disks.  They often seem to be identified and
> > referred to by the target name, however what happens if a snapshot is taken 
> > for
> > 'vda', then the disk's target is changed to 'vdb' and now the snapshot is 
> > to be
> 
> In such case I'd consier it as being different. Similarly as we can't
> really guarantee that the image described by a virStorageSource is the
> same as it was when taking the snapshot. As long as the file is named
> the same you can consider it identical IMO.
> 
> > deleted?  Is there a way to find out that the disk is still there, only 
> > under a
> > different name?
> 
> No and it doesn't seem to be worth doing that.
> 
> > 
> > And as far as comparing chains goes - I know can retrieve the chain for a
> > running disk and I can retrieve what the chain looked like at the point a
> > snapshot was made.  However, how do I establish they are equivalent?  How 
> > can I
> 
> What I meant is that you need to establish that the images you are going
> to merge and the images you are going to merge into are the same
> described by the snapshots and at the same time present in the current
> backing chain of the disk. If they are not in the snapshot metadata
> you'd potentially merge something unwanted, but this is covered by the
> previous paragraphs.
> 
> You need to also verify that they are currently opened by qemu as you
> can't do blockjobs on images which are not part of the chain.
> 
> > tell that snapshots in both chains compare identical in the first place?  Is
> > the snapshot name stable and persistent enough to be useful for this?  Is it
> > enough for chains to be equivalent that the chain at the point of sn

Re: [PATCH 1/6] qemu: fix domain start with corrupted save file

[Pavel Mores]

On Wed, Apr 22, 2020 at 05:04:54PM +0200, Peter Krempa wrote:
> Commit 21ad56e932 introduced a regression where a VM with a corrupted
> save image file would fail to start on the first attempt. This was
> caused by returning a wrong return code as  'fd' was abused to also hold
> the return code.
> 
> Since it's easy to miss this nuance introduce a 'ret' variable for the
> return code and return it's value in the error section.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1791522
> 
> Signed-off-by: Peter Krempa 

Reviewed-by: Pavel Mores 

> ---
>  src/qemu/qemu_driver.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index dfe0adaad8..9a9361949d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6691,6 +6691,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>  bool unlink_corrupt)
>  {
>  int fd = -1;
> +int ret = -1;
>  virQEMUSaveDataPtr data = NULL;
>  virQEMUSaveHeaderPtr header;
>  virDomainDefPtr def = NULL;
> @@ -6726,7 +6727,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>   _("cannot remove corrupt file: %s"),
>   path);
>  } else {
> -fd = -3;
> +ret = -3;
>  }
>  } else {
>  virReportError(VIR_ERR_OPERATION_FAILED,
> @@ -6747,7 +6748,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>   _("cannot remove corrupt file: %s"),
>   path);
>  } else {
> -fd = -3;
> +ret = -3;
>  }
>  goto error;
>  }
> @@ -6816,7 +6817,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
>  virDomainDefFree(def);
>  virQEMUSaveDataFree(data);
>  VIR_FORCE_CLOSE(fd);
> -return -1;
> +return ret;
>  }
> 
>  static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
> -- 
> 2.26.0
> 

Re: [libvirt PATCH v2] qemu: fix domain start with corrupted save file

[Pavel Mores]

On Wed, Apr 22, 2020 at 05:07:01PM +0200, Peter Krempa wrote:
> On Wed, Apr 22, 2020 at 15:15:31 +0200, Pavel Mores wrote:
> > This is to fix
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1791522
> > 
> > With this change, if a domain comes across a corrupted save file during
> > boot it removes the save file and logs a warning but continues to boot
> > normally instead of failing to boot (with a subsequent boot attempt
> > succeeding).
> > 
> > The regression was introduced by 21ad56e932 and this change effectively
> > reverts the relevant part of that commit.
> > ---
> >  src/qemu/qemu_driver.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index e8d47a41cd..2579ef3984 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
> >  *ret_def = def;
> >  *ret_data = data;
> >  
> > + cleanup:
> >  return fd;
> >  
> >   error:
> >  virDomainDefFree(def);
> >  virQEMUSaveDataFree(data);
> >  VIR_FORCE_CLOSE(fd);
> > -return -1;
> > +goto cleanup;
> 
> As pointed out previously this doesn't really help to make it more
> obvious that 'fd' is abused to cary the other return codes here as well.
> 
> I prefer the following fix:
> 
> https://www.redhat.com/archives/libvir-list/2020-April/msg01101.html

Cool, that's incidentally *precisely* the same patch I'd come up with
initially, before I dug in git history this morning and decided to be
conservative and restore Jiří's original fix instead. :-)

pvl

[libvirt PATCH v2] qemu: fix domain start with corrupted save file

[Pavel Mores]

This is to fix

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

With this change, if a domain comes across a corrupted save file during
boot it removes the save file and logs a warning but continues to boot
normally instead of failing to boot (with a subsequent boot attempt
succeeding).

The regression was introduced by 21ad56e932 and this change effectively
reverts the relevant part of that commit.
---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e8d47a41cd..2579ef3984 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 *ret_def = def;
 *ret_data = data;
 
+ cleanup:
 return fd;
 
  error:
 virDomainDefFree(def);
 virQEMUSaveDataFree(data);
 VIR_FORCE_CLOSE(fd);
-return -1;
+goto cleanup;
 }
 
 static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
-- 
2.24.1

Re: [libvirt PATCH] qemu: fix domain start with corrupted save file

[Pavel Mores]

On Wed, Apr 22, 2020 at 01:07:30PM +0200, Peter Krempa wrote:
> On Wed, Apr 22, 2020 at 12:54:26 +0200, Pavel Mores wrote:
> > This is to fix
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1791522
> 
> Please describe the bug in the commit message rather than have users
> open and try understand external links.

It's described in the next paragraph.

> 
> 
> > 
> > With this change, if a domain comes across a corrupted save file during 
> > boot it
> > removes the save file and logs a warning but continues to boot normally 
> > instead
> > of failing to boot (with a subsequent boot attempt succeeding).
> > 
> > The regression was introduced by 21ad56e932 and this change effectively 
> > reverts
> > the relevant part of that commit.
> 
> Please break commit messages at 72 colums as it's common.
> 
> 
> > ---
> >  src/qemu/qemu_driver.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index e8d47a41cd..2579ef3984 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
> >  *ret_def = def;
> >  *ret_data = data;
> >  
> > + cleanup:
> >  return fd;
> >  
> >   error:
> >  virDomainDefFree(def);
> >  virQEMUSaveDataFree(data);
> >  VIR_FORCE_CLOSE(fd);
> > -return -1;
> > +goto cleanup;
> 
> I think this should be fixed properly. 'fd' is serving double duty of
> also reporting the erorrs here and it's not really obvious from this
> hunk. The code above sets 'fd' to a variety of values e.g. -3 on some
> errors. This is really tricky and hard to follow.
> 
> In this case we want a 'ret' variable which is set to the proper value
> and returned on error and fd returned on success or alternatively
> assigned to 'ret' right before returning it. This should make it
> possible to do it even without adding new not really useful labels.
> 

[libvirt PATCH] qemu: fix domain start with corrupted save file

[Pavel Mores]

This is to fix

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

With this change, if a domain comes across a corrupted save file during boot it
removes the save file and logs a warning but continues to boot normally instead
of failing to boot (with a subsequent boot attempt succeeding).

The regression was introduced by 21ad56e932 and this change effectively reverts
the relevant part of that commit.
---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e8d47a41cd..2579ef3984 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6810,13 +6810,14 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver,
 *ret_def = def;
 *ret_data = data;
 
+ cleanup:
 return fd;
 
  error:
 virDomainDefFree(def);
 virQEMUSaveDataFree(data);
 VIR_FORCE_CLOSE(fd);
-return -1;
+goto cleanup;
 }
 
 static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
-- 
2.24.1

Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases

[Pavel Mores]

On Fri, Apr 03, 2020 at 05:56:59PM +0200, Pavel Mores wrote:
> On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote:
> > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote:
> > > +}
> > > +
> > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)
> > > +topPath = disk->src->path;
> > > +else
> > > +topPath = snapdisk->src->path;
> > 
> > You must not use paths for doing this lookup. Paths at best work for
> > local files only and this would make the code not future proof.
> > 
> > Also you want to verify that:
> > - images you want to merge are actually in the backing chain
> > - the backing chain looks as snapshots describe it (e.g you unplug vda
> >   and plug a different chain back)
> 
> Let me check with you how exactly to perform this verification.
> 
> To retrieve the names of the disks included in a snapshot, I can use its
> virDomainSnapshotDef which contains an array of disks
> (virDomainSnapshotDiskDef's) whose 'name' member can be used to identify 
> disks.
> 
> To get the state of the chain at moment the snapshot was created, I can use 
> the
> 'src' member and walk its 'backingStore' list to examine the whole chain.
> 
> To get the currently running configuration, I assume I can use the names of 
> the
> relevant disks (obtained from the snapshot as mentioned above) to get a
> virDomainDiskDefPtr for each of them, whose 'src' member points to a
> virStorageSource that represents the topmost image in the disk's chain.  From
> there, I can walk the 'backingStore' list to get the other images in the chain
> all the way to the base image.
> 
> The verification succeeds if the disk names and their chains in the snapshot
> and the running config correspond.  Is the above correct?
> 
> If so, I'm still not certain how to compare two virStorageSource's.  How is
> identity of two different virStorageSource instances is established?

After having a closer look into this I'm unfortunately even less clear as to
how to perform the checks mentioned in the review.

In addition to the problem of establishing virStorageSource identity, a similar
problem seems to come up with disks.  They often seem to be identified and
referred to by the target name, however what happens if a snapshot is taken for
'vda', then the disk's target is changed to 'vdb' and now the snapshot is to be
deleted?  Is there a way to find out that the disk is still there, only under a
different name?

And as far as comparing chains goes - I know can retrieve the chain for a
running disk and I can retrieve what the chain looked like at the point a
snapshot was made.  However, how do I establish they are equivalent?  How can I
tell that snapshots in both chains compare identical in the first place?  Is
the snapshot name stable and persistent enough to be useful for this?  Is it
enough for chains to be equivalent that the chain at the point of snapshot
creation be a superset of the currently running chain?  Probably yes, provided
snapshots can't be inserted in the middle of a chain - but could that ever
happen?

Thanks in advance for any help with this!

pvl

Re: [PATCH] qemuDomainDefPostParse: Fail if unable to fill machine type

[Pavel Mores]

On Thu, Apr 16, 2020 at 02:33:33PM +0200, Michal Privoznik wrote:
> Previously, we used virCapabilitiesDomainDataLookup() to fill
> machine type in post parse callback if none was provided in the
> domain XML. If machine type couldn't be filled in an error was
> reported. After 4a4132b4625 we've changed it to
> virQEMUCapsGetPreferredMachine() which returns NULL, but we no
> longer report an error and proceed with the post parse callbacks
> processing. This may lead to a crash because the code later on
> assumes def->os.machine is not NULL.
> 
> Fixes: 4a4132b4625
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 91e234d644..98ffd23a71 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4973,6 +4973,14 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>  if (!def->os.machine) {
>  const char *machine = virQEMUCapsGetPreferredMachine(qemuCaps,
>   def->virtType);
> +if (!machine) {
> +virReportError(VIR_ERR_INVALID_ARG,
> +   _("could not get preferred machine for %s 
> type=%s"),
> +   def->emulator,
> +   virDomainVirtTypeToString(def->virtType));
> +return -1;
> +}
> +
>  def->os.machine = g_strdup(machine);
>  }
>  
> -- 
> 2.25.3
> 

Reviewed-by: Pavel Mores 

Re: [PATCH v2] qemu: Revoke access to mirror on failed blockcopy

[Pavel Mores]

On Thu, Apr 16, 2020 at 09:42:46AM +0200, Michal Privoznik wrote:
> When preparing to do a blockcopy, the mirror image is modified so
> that QEMU can access it. For instance, the mirror has seclabels
> set, if it is a NVMe disk it is detached from the host and so on.
> And usually, the restore is done upon successful finish of the
> blockcopy operation. But, if something fails then we need to
> explicitly revoke the access to the mirror image (and thus
> reattach NVMe disk back to the host).
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> diff to v1:
> - Fix call of qemuDomainStorageSourceChainAccessRevoke() so that it is
> called even if data = crdata = NULL.
> 
>  src/qemu/qemu_driver.c | 21 +
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 31f199fdef..dfe0adaad8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  virDomainDiskDefPtr disk = NULL;
>  int ret = -1;
>  bool need_unlink = false;
> +bool need_revoke = false;
>  g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>  const char *format = NULL;
>  bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
> @@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  
>  if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0)
>  goto endjob;
> +need_revoke = true;
>  
>  if (blockdev) {
>  if (mirror_reuse) {
> @@ -18232,14 +18234,17 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  
>   endjob:
>  if (ret < 0 &&
> -virDomainObjIsActive(vm) &&
> -(data || crdata)) {
> -qemuDomainObjEnterMonitor(driver, vm);
> -if (data)
> -qemuBlockStorageSourceChainDetach(priv->mon, data);
> -if (crdata)
> -qemuBlockStorageSourceAttachRollback(priv->mon, 
> crdata->srcdata[0]);
> -ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +virDomainObjIsActive(vm)) {
> +if (data || crdata) {
> +qemuDomainObjEnterMonitor(driver, vm);
> +if (data)
> +qemuBlockStorageSourceChainDetach(priv->mon, data);
> +if (crdata)
> +qemuBlockStorageSourceAttachRollback(priv->mon, 
> crdata->srcdata[0]);
> +ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +}
> +if (need_revoke)
> +    qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror);
>  }
>  if (need_unlink && virStorageFileUnlink(mirror) < 0)
>  VIR_WARN("%s", _("unable to remove just-created copy target"));
> -- 
> 2.25.3
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 01/43] admin: convert virMutex to GMutex

[Pavel Mores]

On Wed, Apr 15, 2020 at 03:58:55PM +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 15, 2020 at 04:11:30PM +0200, Pavel Mores wrote:
> > On Wed, Apr 15, 2020 at 01:55:34PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Apr 14, 2020 at 05:24:50PM +0200, Pavel Mores wrote:
> > > > On Fri, Apr 10, 2020 at 03:54:28PM +0200, Rafael Fonseca wrote:
> > > > > Signed-off-by: Rafael Fonseca 
> > > > > ---
> > > > >  src/admin/admin_server_dispatch.c | 13 -
> > > > >  1 file changed, 4 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/src/admin/admin_server_dispatch.c 
> > > > > b/src/admin/admin_server_dispatch.c
> > > > > index b3da577995..2515528779 100644
> > > > > --- a/src/admin/admin_server_dispatch.c
> > > > > +++ b/src/admin/admin_server_dispatch.c
> > > > > @@ -45,7 +45,7 @@ typedef daemonAdmClientPrivate 
> > > > > *daemonAdmClientPrivatePtr;
> > > > >  /* Separate private data for admin connection */
> > > > >  struct daemonAdmClientPrivate {
> > > > >  /* Just a placeholder, not that there is anything to be locked */
> > > > > -virMutex lock;
> > > > > +GMutex lock;
> > > > >  
> > > > >  virNetDaemonPtr dmn;
> > > > >  };
> > > > > @@ -55,7 +55,7 @@ remoteAdmClientFree(void *data)
> > > > >  {
> > > > >  struct daemonAdmClientPrivate *priv = data;
> > > > >  
> > > > > -virMutexDestroy(>lock);
> > > > > +g_mutex_clear(>lock);
> > > > >  virObjectUnref(priv->dmn);
> > > > >  VIR_FREE(priv);
> > > > >  }
> > > > > @@ -91,11 +91,7 @@ remoteAdmClientNew(virNetServerClientPtr client 
> > > > > G_GNUC_UNUSED,
> > > > >  if (VIR_ALLOC(priv) < 0)
> > > > >  return NULL;
> > > > >  
> > > > > -if (virMutexInit(>lock) < 0) {
> > > > > -VIR_FREE(priv);
> > > > > -virReportSystemError(errno, "%s", _("unable to init mutex"));
> > > > > -return NULL;
> > > > > -}
> > > > > +g_mutex_init(>lock);
> > > > >  
> > > > >  /*
> > > > >   * We don't necessarily need to ref this object right now as 
> > > > > there
> > > > > @@ -167,9 +163,9 @@ adminDispatchConnectOpen(virNetServerPtr server 
> > > > > G_GNUC_UNUSED,
> > > > >  struct daemonAdmClientPrivate *priv =
> > > > >  virNetServerClientGetPrivateData(client);
> > > > >  int ret = -1;
> > > > > +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(>lock);
> > > > >  
> > > > >  VIR_DEBUG("priv=%p dmn=%p", priv, priv->dmn);
> > > > > -virMutexLock(>lock);
> > > > >  
> > > > >  flags = args->flags;
> > > > >  virCheckFlagsGoto(0, cleanup);
> > > > > @@ -178,7 +174,6 @@ adminDispatchConnectOpen(virNetServerPtr server 
> > > > > G_GNUC_UNUSED,
> > > > >   cleanup:
> > > > >  if (ret < 0)
> > > > >  virNetMessageSaveError(rerr);
> > > > > -virMutexUnlock(>lock);
> > > > >  return ret;
> > > > >  }
> > > > >  
> > > > > -- 
> > > > > 2.25.2
> > > > 
> > > > I was wondering why virMutexInit() returns a value whereas 
> > > > g_mutex_init() does
> > > > not, to make sure there weren't any additional adjustments necessary, 
> > > > but it's
> > > > probably OK.
> > > > 
> > > > I mean, it does feel slightly dubious as virMutexInit() fails if
> > > > pthread_mutex_init() fails which can happen under a bunch of seemingly 
> > > > fairly
> > > > realistic scenarios.  I assume g_mutex_init() ultimately calls
> > > > pthread_mutex_init() as well so these scenarios should still apply.
> > > > 
> > > > However, this seems ultimately a problem of glib API designers to 
> > > > decide how
> > > > realistic the scenarios are (at least some of them seem to be related 
> > > > to memory
> > > > allocation which glib solves by aborting) and whether to report them to 
> > > > their
> > > > users, and they made the decision that they made, hopefully for good 
> > > > reasons.
> > > 
> > > Honestly, none of the reasons mutex init can fail are especially
> > > interesting to callers. There's essentially nothing useful callers
> > > can do when a mutex init fails, as its symptomatic of much bigger
> > > problems. Thus I think abort'ing is a reasonable approach. Likewise
> > > for lock/unlock.
> > 
> > Yes, agreed.  At the risk of going off-topic, what makes me wonder is that
> > phtread_mutex_init() POSIX manual page lists also EPERM among the reasons 
> > why
> > the call might fail.  I've never heard of that, let alone encountered it, 
> > and
> > it might not actually be implemented by OS's in practice.  However if EPERM 
> > can
> > happen, then I guess that could be worth reporting to the user.
> 
> Any OS that is crazy enough to report EPERM for initializing mutex is not
> an OS I wish to support :-)

That seems fair enough. :-)

pvl

Re: [PATCH 01/43] admin: convert virMutex to GMutex

[Pavel Mores]

On Wed, Apr 15, 2020 at 01:55:34PM +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 14, 2020 at 05:24:50PM +0200, Pavel Mores wrote:
> > On Fri, Apr 10, 2020 at 03:54:28PM +0200, Rafael Fonseca wrote:
> > > Signed-off-by: Rafael Fonseca 
> > > ---
> > >  src/admin/admin_server_dispatch.c | 13 -
> > >  1 file changed, 4 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/src/admin/admin_server_dispatch.c 
> > > b/src/admin/admin_server_dispatch.c
> > > index b3da577995..2515528779 100644
> > > --- a/src/admin/admin_server_dispatch.c
> > > +++ b/src/admin/admin_server_dispatch.c
> > > @@ -45,7 +45,7 @@ typedef daemonAdmClientPrivate 
> > > *daemonAdmClientPrivatePtr;
> > >  /* Separate private data for admin connection */
> > >  struct daemonAdmClientPrivate {
> > >  /* Just a placeholder, not that there is anything to be locked */
> > > -virMutex lock;
> > > +GMutex lock;
> > >  
> > >  virNetDaemonPtr dmn;
> > >  };
> > > @@ -55,7 +55,7 @@ remoteAdmClientFree(void *data)
> > >  {
> > >  struct daemonAdmClientPrivate *priv = data;
> > >  
> > > -virMutexDestroy(>lock);
> > > +g_mutex_clear(>lock);
> > >  virObjectUnref(priv->dmn);
> > >  VIR_FREE(priv);
> > >  }
> > > @@ -91,11 +91,7 @@ remoteAdmClientNew(virNetServerClientPtr client 
> > > G_GNUC_UNUSED,
> > >  if (VIR_ALLOC(priv) < 0)
> > >  return NULL;
> > >  
> > > -if (virMutexInit(>lock) < 0) {
> > > -VIR_FREE(priv);
> > > -virReportSystemError(errno, "%s", _("unable to init mutex"));
> > > -return NULL;
> > > -}
> > > +g_mutex_init(>lock);
> > >  
> > >  /*
> > >   * We don't necessarily need to ref this object right now as there
> > > @@ -167,9 +163,9 @@ adminDispatchConnectOpen(virNetServerPtr server 
> > > G_GNUC_UNUSED,
> > >  struct daemonAdmClientPrivate *priv =
> > >  virNetServerClientGetPrivateData(client);
> > >  int ret = -1;
> > > +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(>lock);
> > >  
> > >  VIR_DEBUG("priv=%p dmn=%p", priv, priv->dmn);
> > > -virMutexLock(>lock);
> > >  
> > >  flags = args->flags;
> > >  virCheckFlagsGoto(0, cleanup);
> > > @@ -178,7 +174,6 @@ adminDispatchConnectOpen(virNetServerPtr server 
> > > G_GNUC_UNUSED,
> > >   cleanup:
> > >  if (ret < 0)
> > >  virNetMessageSaveError(rerr);
> > > -virMutexUnlock(>lock);
> > >  return ret;
> > >  }
> > >  
> > > -- 
> > > 2.25.2
> > 
> > I was wondering why virMutexInit() returns a value whereas g_mutex_init() 
> > does
> > not, to make sure there weren't any additional adjustments necessary, but 
> > it's
> > probably OK.
> > 
> > I mean, it does feel slightly dubious as virMutexInit() fails if
> > pthread_mutex_init() fails which can happen under a bunch of seemingly 
> > fairly
> > realistic scenarios.  I assume g_mutex_init() ultimately calls
> > pthread_mutex_init() as well so these scenarios should still apply.
> > 
> > However, this seems ultimately a problem of glib API designers to decide how
> > realistic the scenarios are (at least some of them seem to be related to 
> > memory
> > allocation which glib solves by aborting) and whether to report them to 
> > their
> > users, and they made the decision that they made, hopefully for good 
> > reasons.
> 
> Honestly, none of the reasons mutex init can fail are especially
> interesting to callers. There's essentially nothing useful callers
> can do when a mutex init fails, as its symptomatic of much bigger
> problems. Thus I think abort'ing is a reasonable approach. Likewise
> for lock/unlock.

Yes, agreed.  At the risk of going off-topic, what makes me wonder is that
phtread_mutex_init() POSIX manual page lists also EPERM among the reasons why
the call might fail.  I've never heard of that, let alone encountered it, and
it might not actually be implemented by OS's in practice.  However if EPERM can
happen, then I guess that could be worth reporting to the user.

Well it's not like we need to solve this problem on this list anyway...

pvl

Re: [PATCH 02/43] bhyve: convert virMutex to GMutex

[Pavel Mores]

On Wed, Apr 15, 2020 at 01:59:46PM +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 15, 2020 at 02:15:35PM +0200, Pavel Mores wrote:
> > On Tue, Apr 14, 2020 at 07:05:03PM +0200, Rafael Fonseca wrote:
> > > On Tue, Apr 14, 2020 at 6:06 PM Pavel Mores  wrote:
> > > >
> > > > By the way, the approach taken here with bhyveDriver{Lock,Unlock}() 
> > > > might make
> > > > sense with the whole series - implement e.g. virMutexInit() in terms of
> > > > g_mutex_init() in the first phase and only then replace the actual
> > > > virMutexInit() calls if considered beneficial...
> > > 
> > > So you mean one patch doing 's/virMutex/GMutex' and then inside
> > > virMutex*() we call the g_mutex_*() equivalent? And maybe make
> > > virMutex*() `inline`?
> > 
> > Yes - I mean, I'm not familiar enough with this to be sure off-hand that 
> > just
> > doing a literal find & replace would work with no undesired side-effects, 
> > but
> > conceptually yes, that's the idea.
> > 
> > That's just a thought though - taking that approach would have broken the
> > refactor into two more manageable & testable chunks but seeing as you've 
> > done
> > the hard work already, there's no need to rework the series just because of 
> > me.
> > :-)
> 
> Replacing the virMutex calls with GMutex APis in all callers is the
> desirable approach.  The goal of using GLib APIs is to remove any
> libvirt specfic APIs which duplicate GLib.  Thus re-writing virMutex
> APIs impls to call GMutex is just delaying the desired end state where
> virMutex ceases to exist.

Sure, I agree with the goal, no question about that.  I was just thinking about
getting there in two steps (with no delay between them implied) which might
have been at least easier to review.  Also bisecting would be easier that way
if we later suspect that there might have been an unexpected change of
behaviour after all, possibly related to the migration to glib sync primitives.

Anyway, the work has been pretty much done already so the point seems moot.

Thanks,

pvl

Re: [PATCH 03/43] conf: nwfilter_ipaddrmap: convert virMutex to GMutex

[Pavel Mores]

On Sat, Apr 11, 2020 at 08:15:15PM +0200, Rafael Fonseca wrote:
> On Fri, Apr 10, 2020 at 7:06 PM Laine Stump  wrote:
> > > Beyond that, why not just use the G_*() macros for *all* locks in
> > > libvirt instead of only using them in cases of static locks? It seems
> > > strange to use the convenience macros in some cases and the raw
> > > functions in others. (I'm looking at this with 0 experience using the
> > > Glib locks, so maybe there's something basic I'm just not aware of -
> > > maybe something necessary is missing from the G_LOCK() macros?).
> >
> >
> > Okay, I already see that the G_LOCK macros don't cover everything - no
> > recursive mutexes and no RW mutexes for example. Too bad - it would be
> > good to be consistent.
> 
> Yes, that's one issue. Another is: how do you use those macros with
> locks inside structs? You can't do `G_LOCK(obj->parent.lock)` because
> it'll result in `g_mutex_lock(__obj->parent.lock_lock)` which is
> wrong. You'd have to use the raw function
> `g_mutex_lock(obj->parent.LOCK_NAME(lock))` anyway, which imho, is
> even worse than `g_mutex_lock(>parent.lock)`. The same issue
> happens when using mutexes with conditions: `g_cond_wait(cond,
> obj->parent.LOCK_NAME(lock))
> ` instead of just `g_cond_wait(cond, obj->parent.lock)`. So they work
> better for statically-defined locks
> 
> I don't mind doing whichever you guys prefer, just let me know.

Looking at the source code, the name mangling is pretty much all that the
G_LOCK_DEFINE* macros do.  So - besides some logging - the only advantage to
using them is that you don't have to mangle the lock names manually and can use
names of existing variables as the macros' arguments.

Considering the above, I'd say either use the macros and don't mangle the lock
names in their arguments manually, or don't use the macros.  If consistent
style is a priority I'd lean towards raw functions - unlike the macros, they
can be used everywhere, and having to mangle the lock names by hand doesn't
seem a huge burden to me.  We do loose the logging that the macros do but in my
experience, mutex logging often doesn't turn out as useful in practice as it
might first appear...

pvl

Re: [PATCH 02/43] bhyve: convert virMutex to GMutex

[Pavel Mores]

On Tue, Apr 14, 2020 at 07:05:03PM +0200, Rafael Fonseca wrote:
> On Tue, Apr 14, 2020 at 6:06 PM Pavel Mores  wrote:
> >
> > By the way, the approach taken here with bhyveDriver{Lock,Unlock}() might 
> > make
> > sense with the whole series - implement e.g. virMutexInit() in terms of
> > g_mutex_init() in the first phase and only then replace the actual
> > virMutexInit() calls if considered beneficial...
> 
> So you mean one patch doing 's/virMutex/GMutex' and then inside
> virMutex*() we call the g_mutex_*() equivalent? And maybe make
> virMutex*() `inline`?

Yes - I mean, I'm not familiar enough with this to be sure off-hand that just
doing a literal find & replace would work with no undesired side-effects, but
conceptually yes, that's the idea.

That's just a thought though - taking that approach would have broken the
refactor into two more manageable & testable chunks but seeing as you've done
the hard work already, there's no need to rework the series just because of me.
:-)

pvl

Re: [PATCH] qemu: Revoke access to mirror on failed blockcopy

[Pavel Mores]

On Tue, Apr 14, 2020 at 11:32:08AM +0200, Michal Privoznik wrote:
> When preparing to do a blockcopy, the mirror image is modified so
> that QEMU can access it. For instance, the mirror has seclabels
> set, if it is a NVMe disk it is detached from the host and so on.
> And usually, the restore is done upon successful finish of the
> blockcopy operation. But, if something fails then we need to
> explicitly revoke the access to the mirror image (and thus
> reattach NVMe disk back to the host).
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_driver.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 31f199fdef..9475235f01 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  virDomainDiskDefPtr disk = NULL;
>  int ret = -1;
>  bool need_unlink = false;
> +bool need_revoke = false;
>  g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>  const char *format = NULL;
>  bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
> @@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  
>  if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0)
>  goto endjob;
> +need_revoke = true;
>  
>  if (blockdev) {
>  if (mirror_reuse) {
> @@ -18240,6 +18242,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>  if (crdata)
>  qemuBlockStorageSourceAttachRollback(priv->mon, 
> crdata->srcdata[0]);
>  ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +if (need_revoke)
> +qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror);
>  }
>  if (need_unlink && virStorageFileUnlink(mirror) < 0)
>  VIR_WARN("%s", _("unable to remove just-created copy target"));

Is the revocation code correctly placed though?  I mean, it seems to follow
from the patch that we need to revoke as soon as
qemuDomainStorageSourceChainAccessAllow() succeeds.  However, the revocation
call is conditioned by there being 'data' or 'crdata' (among other things).

What happens if qemuDomainStorageSourceChainAccessAllow() succeeds but the
first subsequent attempt to retrieve 'data' or 'crdata' (whichever comes first)
fails?  These failures are handled by 'goto endjob' which reaches the clean-up
section apparently with both 'data' and 'crdata' still being NULL but
'need_revoke' true. If the above is correct, that would mean the 'if
(need_revoke)' code is never reached and no revocation performed.

What am I missing?

pvl

Re: [PATCH 20/43] network: bridge_driver: convert virMutex to GMutex

[Pavel Mores]

On Fri, Apr 10, 2020 at 03:54:47PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/network/bridge_driver.c  | 11 ---
>  src/network/bridge_driver_platform.h |  2 +-
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index f06099297a..2eaab9c667 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -101,14 +101,14 @@ networkGetDriver(void)
>  static void
>  networkDriverLock(virNetworkDriverStatePtr driver)
>  {
> -virMutexLock(>lock);
> +g_mutex_lock(>lock);
>  }
>  
>  
>  static void
>  networkDriverUnlock(virNetworkDriverStatePtr driver)
>  {
> -virMutexUnlock(>lock);
> +g_mutex_unlock(>lock);
>  }
>  
>  
> @@ -726,10 +726,7 @@ networkStateInitialize(bool privileged,
>  goto error;
>  
>  network_driver->lockFD = -1;
> -if (virMutexInit(_driver->lock) < 0) {
> -VIR_FREE(network_driver);
> -goto error;
> -}
> +g_mutex_init(_driver->lock);
>  
>  network_driver->privileged = privileged;
>  
> @@ -907,7 +904,7 @@ networkStateCleanup(void)
>  
>  virObjectUnref(network_driver->dnsmasqCaps);
>  
> -virMutexDestroy(_driver->lock);
> +g_mutex_clear(_driver->lock);
>  
>  VIR_FREE(network_driver);
>  
> diff --git a/src/network/bridge_driver_platform.h 
> b/src/network/bridge_driver_platform.h
> index 169417a6c0..6528bf6647 100644
> --- a/src/network/bridge_driver_platform.h
> +++ b/src/network/bridge_driver_platform.h
> @@ -29,7 +29,7 @@
>  
>  /* Main driver state */
>  struct _virNetworkDriverState {
> -virMutex lock;
> +GMutex lock;
>  
>  /* Read-only */
>  bool privileged;
> -- 
> 2.25.2
> 
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 06/43] util: virtpm: convert virMutex to GMutex

[Pavel Mores]

On Fri, Apr 10, 2020 at 03:54:33PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/util/virtpm.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/src/util/virtpm.c b/src/util/virtpm.c
> index c734bf941a..5fd6396f2f 100644
> --- a/src/util/virtpm.c
> +++ b/src/util/virtpm.c
> @@ -91,7 +91,7 @@ virTPMCreateCancelPath(const char *devpath)
>   * executables for the swtpm; to be found on the host along with
>   * capabilties bitmap
>   */
> -static virMutex swtpm_tools_lock = VIR_MUTEX_INITIALIZER;
> +G_LOCK_DEFINE_STATIC(swtpm_tools_lock);
>  static char *swtpm_path;
>  static struct stat swtpm_stat;
>  static virBitmapPtr swtpm_caps;
> @@ -113,9 +113,9 @@ virTPMGetSwtpm(void)
>  if (!swtpm_path && virTPMEmulatorInit() < 0)
>  return NULL;
>  
> -virMutexLock(_tools_lock);
> +G_LOCK(swtpm_tools_lock);
>  s = g_strdup(swtpm_path);
> -virMutexUnlock(_tools_lock);
> +G_UNLOCK(swtpm_tools_lock);
>  
>  return s;
>  }
> @@ -128,9 +128,9 @@ virTPMGetSwtpmSetup(void)
>  if (!swtpm_setup && virTPMEmulatorInit() < 0)
>  return NULL;
>  
> -virMutexLock(_tools_lock);
> +G_LOCK(swtpm_tools_lock);
>  s = g_strdup(swtpm_setup);
> -virMutexUnlock(_tools_lock);
> +G_UNLOCK(swtpm_tools_lock);
>  
>  return s;
>  }
> @@ -143,9 +143,9 @@ virTPMGetSwtpmIoctl(void)
>  if (!swtpm_ioctl && virTPMEmulatorInit() < 0)
>  return NULL;
>  
> -virMutexLock(_tools_lock);
> +G_LOCK(swtpm_tools_lock);
>  s = g_strdup(swtpm_ioctl);
> -virMutexUnlock(_tools_lock);
> +G_UNLOCK(swtpm_tools_lock);
>  
>  return s;
>  }
> @@ -284,7 +284,7 @@ virTPMEmulatorInit(void)
>  };
>  size_t i;
>  
> -virMutexLock(_tools_lock);
> +G_LOCK(swtpm_tools_lock);
>  
>  for (i = 0; i < G_N_ELEMENTS(prgs); i++) {
>  g_autofree char *path = NULL;
> @@ -341,7 +341,7 @@ virTPMEmulatorInit(void)
>  ret = 0;
>  
>   cleanup:
> -virMutexUnlock(_tools_lock);
> +G_UNLOCK(swtpm_tools_lock);
>  
>  return ret;
>  }
> -- 
> 2.25.2
> 
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 04/43] conf: virchrdev: convert virMutex to GMutex

[Pavel Mores]

On Fri, Apr 10, 2020 at 03:54:31PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/conf/virchrdev.c | 27 +++
>  1 file changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c
> index 800e82869e..8280f8e188 100644
> --- a/src/conf/virchrdev.c
> +++ b/src/conf/virchrdev.c
> @@ -44,7 +44,7 @@ VIR_LOG_INIT("conf.chrdev");
>  /* structure holding information about character devices
>   * open in a given domain */
>  struct _virChrdevs {
> -virMutex lock;
> +GMutex lock;
>  virHashTablePtr hash;
>  };
>  
> @@ -238,12 +238,10 @@ static void virChrdevFDStreamCloseCb(virStreamPtr st 
> G_GNUC_UNUSED,
>void *opaque)
>  {
>  virChrdevStreamInfoPtr priv = opaque;
> -virMutexLock(>devs->lock);
> +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(>devs->lock);
>  
>  /* remove entry from hash */
>  virHashRemoveEntry(priv->devs->hash, priv->path);
> -
> -virMutexUnlock(>devs->lock);
>  }
>  
>  /**
> @@ -258,12 +256,7 @@ virChrdevsPtr virChrdevAlloc(void)
>  if (VIR_ALLOC(devs) < 0)
>  return NULL;
>  
> -if (virMutexInit(>lock) < 0) {
> -virReportSystemError(errno, "%s",
> - _("Unable to init device stream mutex"));
> -VIR_FREE(devs);
> -return NULL;
> -}
> +g_mutex_init(>lock);
>  
>  /* there will hardly be any devices most of the time, the hash
>   * does not have to be huge */
> @@ -299,11 +292,11 @@ void virChrdevFree(virChrdevsPtr devs)
>  if (!devs)
>  return;
>  
> -virMutexLock(>lock);
> +g_mutex_lock(>lock);
>  virHashForEach(devs->hash, virChrdevFreeClearCallbacks, NULL);
>  virHashFree(devs->hash);
> -virMutexUnlock(>lock);
> -virMutexDestroy(>lock);
> +g_mutex_unlock(>lock);
> +g_mutex_clear(>lock);
>  
>  VIR_FREE(devs);
>  }
> @@ -334,6 +327,7 @@ int virChrdevOpen(virChrdevsPtr devs,
>  char *path;
>  int ret;
>  bool added = false;
> +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(>lock);
>  
>  switch (source->type) {
>  case VIR_DOMAIN_CHR_TYPE_PTY:
> @@ -354,12 +348,9 @@ int virChrdevOpen(virChrdevsPtr devs,
>  return -1;
>  }
>  
> -virMutexLock(>lock);
> -
>  if ((ent = virHashLookup(devs->hash, path))) {
>  if (!force) {
>   /* entry found, device is busy */
> -virMutexUnlock(>lock);
>  return 1;
> } else {
> /* terminate existing connection */
> @@ -378,13 +369,11 @@ int virChrdevOpen(virChrdevsPtr devs,
>  
>  /* create the lock file */
>  if ((ret = virChrdevLockFileCreate(path)) < 0) {
> -virMutexUnlock(>lock);
>  return ret;
>  }
>  
>  /* obtain a reference to the stream */
>  if (virStreamRef(st) < 0) {
> -virMutexUnlock(>lock);
>  return -1;
>  }
>  
> @@ -428,7 +417,6 @@ int virChrdevOpen(virChrdevsPtr devs,
>    cbdata,
>virChrdevFDStreamCloseCbFree);
>  
> -virMutexUnlock(>lock);
>  return 0;
>  
>   error:
> @@ -440,7 +428,6 @@ int virChrdevOpen(virChrdevsPtr devs,
>  if (cbdata)
>  VIR_FREE(cbdata->path);
>  VIR_FREE(cbdata);
> -virMutexUnlock(>lock);
>  virChrdevHashEntryFree(ent);
>  return -1;
>  }
> -- 
> 2.25.2
> 
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 02/43] bhyve: convert virMutex to GMutex

[Pavel Mores]

On Fri, Apr 10, 2020 at 03:54:29PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/bhyve/bhyve_driver.c | 11 ---
>  src/bhyve/bhyve_utils.h  |  2 +-
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> index b6204c7fb9..6e9a79cb52 100644
> --- a/src/bhyve/bhyve_driver.c
> +++ b/src/bhyve/bhyve_driver.c
> @@ -73,13 +73,13 @@ bhyveConnPtr bhyve_driver = NULL;
>  void
>  bhyveDriverLock(bhyveConnPtr driver)
>  {
> -virMutexLock(>lock);
> +g_mutex_lock(>lock);
>  }
>  
>  void
>  bhyveDriverUnlock(bhyveConnPtr driver)
>  {
> -virMutexUnlock(>lock);
> +g_mutex_unlock(>lock);
>  }
>  
>  static int
> @@ -1199,7 +1199,7 @@ bhyveStateCleanup(void)
>  if (bhyve_driver->lockFD != -1)
>  virPidFileRelease(BHYVE_STATE_DIR, "driver", bhyve_driver->lockFD);
>  
> -virMutexDestroy(_driver->lock);
> +g_mutex_clear(_driver->lock);
>  VIR_FREE(bhyve_driver);
>  
>  return 0;
> @@ -1228,10 +1228,7 @@ bhyveStateInitialize(bool privileged,
>  return VIR_DRV_STATE_INIT_ERROR;
>  
>  bhyve_driver->lockFD = -1;
> -if (virMutexInit(_driver->lock) < 0) {
> -VIR_FREE(bhyve_driver);
> -return VIR_DRV_STATE_INIT_ERROR;
> -}
> +g_mutex_init(_driver->lock);
>  
>  if (!(bhyve_driver->closeCallbacks = virCloseCallbacksNew()))
>  goto cleanup;
> diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h
> index f3e80b6121..a92ecb48c4 100644
> --- a/src/bhyve/bhyve_utils.h
> +++ b/src/bhyve/bhyve_utils.h
> @@ -44,7 +44,7 @@ struct _virBhyveDriverConfig {
>  };
>  
>  struct _bhyveConn {
> -virMutex lock;
> +GMutex lock;
>  
>  virBhyveDriverConfigPtr config;
>  
> -- 
> 2.25.2
> 
> 

By the way, the approach taken here with bhyveDriver{Lock,Unlock}() might make
sense with the whole series - implement e.g. virMutexInit() in terms of
g_mutex_init() in the first phase and only then replace the actual
virMutexInit() calls if considered beneficial...

Reviewed-by: Pavel Mores 

Re: [PATCH 01/43] admin: convert virMutex to GMutex

[Pavel Mores]

On Fri, Apr 10, 2020 at 03:54:28PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca 
> ---
>  src/admin/admin_server_dispatch.c | 13 -
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/src/admin/admin_server_dispatch.c 
> b/src/admin/admin_server_dispatch.c
> index b3da577995..2515528779 100644
> --- a/src/admin/admin_server_dispatch.c
> +++ b/src/admin/admin_server_dispatch.c
> @@ -45,7 +45,7 @@ typedef daemonAdmClientPrivate *daemonAdmClientPrivatePtr;
>  /* Separate private data for admin connection */
>  struct daemonAdmClientPrivate {
>  /* Just a placeholder, not that there is anything to be locked */
> -virMutex lock;
> +GMutex lock;
>  
>  virNetDaemonPtr dmn;
>  };
> @@ -55,7 +55,7 @@ remoteAdmClientFree(void *data)
>  {
>  struct daemonAdmClientPrivate *priv = data;
>  
> -virMutexDestroy(>lock);
> +g_mutex_clear(>lock);
>  virObjectUnref(priv->dmn);
>  VIR_FREE(priv);
>  }
> @@ -91,11 +91,7 @@ remoteAdmClientNew(virNetServerClientPtr client 
> G_GNUC_UNUSED,
>  if (VIR_ALLOC(priv) < 0)
>  return NULL;
>  
> -if (virMutexInit(>lock) < 0) {
> -VIR_FREE(priv);
> -virReportSystemError(errno, "%s", _("unable to init mutex"));
> -return NULL;
> -}
> +g_mutex_init(>lock);
>  
>  /*
>   * We don't necessarily need to ref this object right now as there
> @@ -167,9 +163,9 @@ adminDispatchConnectOpen(virNetServerPtr server 
> G_GNUC_UNUSED,
>  struct daemonAdmClientPrivate *priv =
>  virNetServerClientGetPrivateData(client);
>  int ret = -1;
> +g_autoptr(GMutexLocker) locker = g_mutex_locker_new(>lock);
>  
>  VIR_DEBUG("priv=%p dmn=%p", priv, priv->dmn);
> -virMutexLock(>lock);
>  
>  flags = args->flags;
>  virCheckFlagsGoto(0, cleanup);
> @@ -178,7 +174,6 @@ adminDispatchConnectOpen(virNetServerPtr server 
> G_GNUC_UNUSED,
>   cleanup:
>  if (ret < 0)
>  virNetMessageSaveError(rerr);
> -virMutexUnlock(>lock);
>  return ret;
>  }
>  
> -- 
> 2.25.2

I was wondering why virMutexInit() returns a value whereas g_mutex_init() does
not, to make sure there weren't any additional adjustments necessary, but it's
probably OK.

I mean, it does feel slightly dubious as virMutexInit() fails if
pthread_mutex_init() fails which can happen under a bunch of seemingly fairly
realistic scenarios.  I assume g_mutex_init() ultimately calls
pthread_mutex_init() as well so these scenarios should still apply.

However, this seems ultimately a problem of glib API designers to decide how
realistic the scenarios are (at least some of them seem to be related to memory
allocation which glib solves by aborting) and whether to report them to their
users, and they made the decision that they made, hopefully for good reasons.

Reviewed-by: Pavel Mores 

Re: [PATCH] storage: use local dom_disk parameter

[Pavel Mores]

On Tue, Apr 07, 2020 at 08:10:29PM +0800, Yi Li wrote:
> replace vm->def->disks[i]  with dom_disk parameter
> 
> Signed-off-by: Yi Li 
> ---
>  src/qemu/qemu_driver.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index daa3cb397d..27f43124ac 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14841,14 +14841,13 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
>active) < 0)
>  return -1;
>  
> -if (vm->def->disks[i]->src->format > 0 &&
> -vm->def->disks[i]->src->format != VIR_STORAGE_FILE_QCOW2) {
> +if (dom_disk->src->format > 0 &&
> +dom_disk->src->format != VIR_STORAGE_FILE_QCOW2) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("internal snapshot for disk %s unsupported "
>   "for storage type %s"),
> disk->name,
> -   virStorageFileFormatTypeToString(
> -   vm->def->disks[i]->src->format));
> +       
> virStorageFileFormatTypeToString(dom_disk->src->format));
>  return -1;
>  }
>  break;
> -- 
> 2.13.6
> 

Reviewed-by: Pavel Mores 

Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases

[Pavel Mores]

On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote:
> On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote:
> > +}
> > +
> > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)
> > +topPath = disk->src->path;
> > +else
> > +topPath = snapdisk->src->path;
> 
> You must not use paths for doing this lookup. Paths at best work for
> local files only and this would make the code not future proof.
> 
> Also you want to verify that:
> - images you want to merge are actually in the backing chain
> - the backing chain looks as snapshots describe it (e.g you unplug vda
>   and plug a different chain back)

Let me check with you how exactly to perform this verification.

To retrieve the names of the disks included in a snapshot, I can use its
virDomainSnapshotDef which contains an array of disks
(virDomainSnapshotDiskDef's) whose 'name' member can be used to identify disks.

To get the state of the chain at moment the snapshot was created, I can use the
'src' member and walk its 'backingStore' list to examine the whole chain.

To get the currently running configuration, I assume I can use the names of the
relevant disks (obtained from the snapshot as mentioned above) to get a
virDomainDiskDefPtr for each of them, whose 'src' member points to a
virStorageSource that represents the topmost image in the disk's chain.  From
there, I can walk the 'backingStore' list to get the other images in the chain
all the way to the base image.

The verification succeeds if the disk names and their chains in the snapshot
and the running config correspond.  Is the above correct?

If so, I'm still not certain how to compare two virStorageSource's.  How is
identity of two different virStorageSource instances is established?

Thanks!

pvl

Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases

[Pavel Mores]

On Fri, Apr 03, 2020 at 02:36:35PM +0200, Peter Krempa wrote:
> On Fri, Apr 03, 2020 at 14:34:29 +0200, Pavel Mores wrote:
> > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote:
> > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote:
> > > 
> > > > +}
> > > > +
> > > > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)
> > > > +topPath = disk->src->path;
> > > > +else
> > > > +topPath = snapdisk->src->path;
> > > 
> > > You must not use paths for doing this lookup. Paths at best work for
> > > local files only and this would make the code not future proof.
> > > 
> > > Also you want to verify that:
> > > - images you want to merge are actually in the backing chain
> > > - the backing chain looks as snapshots describe it (e.g you unplug vda
> > >   and plug a different chain back)
> > > 
> > > Doing the validation above will necessarily give you a
> > > virStorageSourcePtr for the appropriate member of the backing chain and
> > > that one should be used as argument for the commit operation.
> > 
> > I'm afraid I'm not following this.  qemuDomainBlockCommitImpl(), just like
> > qemuDomainBlockCommit() take paths so that's what I'm passing them.  What do
> > you mean, I should use virStorageSourcePtr as argument for the commit
> > operation?
> 
> I'm saying you should not use paths at all. Use virStorageSource
> directly. If you look inside qemuDomainBlockCommitImpl there are calls
> which take path and look up the virStorageSource.
> 
> Specifically e.g. for NBD disks path may be NULL, thus must not be used
> if your code has to work for everything. It's okay only to use them when
> passed by user, but not internally.

Alright, so you mean a further refactor of qemuDomainBlockCommitImpl() is
needed.  Thanks, I'll look into it.

pvl

Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases

[Pavel Mores]

On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote:
> On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote:
> 
> > +}
> > +
> > +if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)
> > +topPath = disk->src->path;
> > +else
> > +topPath = snapdisk->src->path;
> 
> You must not use paths for doing this lookup. Paths at best work for
> local files only and this would make the code not future proof.
> 
> Also you want to verify that:
> - images you want to merge are actually in the backing chain
> - the backing chain looks as snapshots describe it (e.g you unplug vda
>   and plug a different chain back)
> 
> Doing the validation above will necessarily give you a
> virStorageSourcePtr for the appropriate member of the backing chain and
> that one should be used as argument for the commit operation.

I'm afraid I'm not following this.  qemuDomainBlockCommitImpl(), just like
qemuDomainBlockCommit() take paths so that's what I'm passing them.  What do
you mean, I should use virStorageSourcePtr as argument for the commit
operation?

pvl

Re: [PATCH 0/2] Optimize initialization of storage files

[Pavel Mores]

On Thu, Mar 26, 2020 at 12:18:01PM +0100, Peter Krempa wrote:
> See 2/2.
> 
> Peter Krempa (2):
>   qemuSecurityChownCallback: Remove 'cleanup' section
>   qemuSecurityChownCallback: Don't initialize storage file subsystem for
> local file
> 
>  src/qemu/qemu_driver.c | 31 +--
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 3/3] qemuBlockJobRefreshJobs: Warn readers that 'job' may be invalid after update

[Pavel Mores]

On Thu, Mar 26, 2020 at 01:39:41PM +0100, Peter Krempa wrote:
> Add a comment noting that job update can cause the pointer to be invalid
> and thus should not be accessed after.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_blockjob.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 6576f8721f..2032c0c1c5 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -571,6 +571,7 @@ qemuBlockJobRefreshJobs(virQEMUDriverPtr driver,
> 
>  if (job->newstate != -1)
>  qemuBlockJobUpdate(vm, job, QEMU_ASYNC_JOB_NONE);
> +/* 'job' may be invalid after this update */
>  }
> 
>  /* remove data for job which qemu didn't report (the algorithm is
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 1/3] qemu: migration: Don't use return value of qemuBlockJobUpdate

[Pavel Mores]

On Thu, Mar 26, 2020 at 01:39:39PM +0100, Peter Krempa wrote:
> Upcoming patch will remove it.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_migration.c | 17 +++--
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 8a1801d408..bc280e856a 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -510,7 +510,6 @@ qemuMigrationSrcNBDStorageCopyReady(virDomainObjPtr vm,
>  {
>  size_t i;
>  size_t notReady = 0;
> -int status;
> 
>  for (i = 0; i < vm->def->ndisks; i++) {
>  virDomainDiskDefPtr disk = vm->def->disks[i];
> @@ -526,8 +525,8 @@ qemuMigrationSrcNBDStorageCopyReady(virDomainObjPtr vm,
>  return -1;
>  }
> 
> -status = qemuBlockJobUpdate(vm, job, asyncJob);
> -if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) {
> +qemuBlockJobUpdate(vm, job, asyncJob);
> +if (job->state == VIR_DOMAIN_BLOCK_JOB_FAILED) {
>  qemuMigrationNBDReportMirrorError(job, disk->dst);
>  virObjectUnref(job);
>  return -1;
> @@ -567,7 +566,6 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm,
>  size_t i;
>  size_t active = 0;
>  size_t completed = 0;
> -int status;
>  bool failed = false;
> 
>   retry:
> @@ -582,8 +580,8 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm,
>  if (!(job = qemuBlockJobDiskGetJob(disk)))
>  continue;
> 
> -status = qemuBlockJobUpdate(vm, job, asyncJob);
> -switch (status) {
> +qemuBlockJobUpdate(vm, job, asyncJob);
> +switch (job->state) {
>  case VIR_DOMAIN_BLOCK_JOB_FAILED:
>  if (check) {
>  qemuMigrationNBDReportMirrorError(job, disk->dst);
> @@ -599,7 +597,7 @@ qemuMigrationSrcNBDCopyCancelled(virDomainObjPtr vm,
>  active++;
>  }
> 
> -if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED)
> +if (job->state == VIR_DOMAIN_BLOCK_JOB_COMPLETED)
>  completed++;
> 
>  virObjectUnref(job);
> @@ -650,11 +648,10 @@ qemuMigrationSrcNBDCopyCancelOne(virQEMUDriverPtr 
> driver,
>   qemuDomainAsyncJob asyncJob)
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
> -int status;
>  int rv;
> 
> -status = qemuBlockJobUpdate(vm, job, asyncJob);
> -switch (status) {
> +qemuBlockJobUpdate(vm, job, asyncJob);
> +switch (job->state) {
>  case VIR_DOMAIN_BLOCK_JOB_FAILED:
>  case VIR_DOMAIN_BLOCK_JOB_CANCELED:
>  if (failNoJob) {
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 2/3] qemuBlockJobUpdate: Remove return value

[Pavel Mores]

On Thu, Mar 26, 2020 at 01:39:40PM +0100, Peter Krempa wrote:
> No callers use it any more. Additionally if qemuBlockJobUpdate was
> called with the last reference of the job e.g. in
> qemuBlockJobRefreshJobs, the reading of the job state would happen from
> freed memory.
> 
> Reported-by: Pavel Mores 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_blockjob.c | 8 ++--
>  src/qemu/qemu_blockjob.h | 7 ---
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 21a043d369..6576f8721f 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -1658,10 +1658,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
>   *
>   * Update disk's mirror state in response to a block job event stored in
>   * blockJobStatus by qemuProcessHandleBlockJob event handler.
> - *
> - * Returns the block job event processed or -1 if there was no pending event.
>   */
> -int
> +void
>  qemuBlockJobUpdate(virDomainObjPtr vm,
> qemuBlockJobDataPtr job,
> int asyncJob)
> @@ -1669,14 +1667,12 @@ qemuBlockJobUpdate(virDomainObjPtr vm,
>  qemuDomainObjPrivatePtr priv = vm->privateData;
> 
>  if (job->newstate == -1)
> -return -1;
> +return;
> 
>  if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV))
>  qemuBlockJobEventProcess(priv->driver, vm, job, asyncJob);
>  else
>  qemuBlockJobEventProcessLegacy(priv->driver, vm, job, asyncJob);
> -
> -return job->state;
>  }
> 
> 
> diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
> index 9264c70217..19498b5bd8 100644
> --- a/src/qemu/qemu_blockjob.h
> +++ b/src/qemu/qemu_blockjob.h
> @@ -232,9 +232,10 @@ int
>  qemuBlockJobRefreshJobs(virQEMUDriverPtr driver,
>  virDomainObjPtr vm);
> 
> -int qemuBlockJobUpdate(virDomainObjPtr vm,
> -   qemuBlockJobDataPtr job,
> -   int asyncJob);
> +void
> +qemuBlockJobUpdate(virDomainObjPtr vm,
> +   qemuBlockJobDataPtr job,
> +   int asyncJob);
> 
>  void qemuBlockJobSyncBegin(qemuBlockJobDataPtr job);
>  void qemuBlockJobSyncEnd(virDomainObjPtr vm,
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

[libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases

[Pavel Mores]

Works by blockcommitting the snapshot to be deleted into its parent.  Handles
both deleting children as well and deleting children only (commits the
children into the snapshot referred to by snapshot-delete argument).  If the
necessary block commit operation turns out to be active (the snapshot referred
to by snapshot-delete argument is current, or deleting children is requested)
pivot is performed as well.

This implemetation is limited to the straightforward case which assumes no
branching snapshot chains below the snapshot to be deleted, and the current
snapshot has to be the leaf of the (linear) chain starting at the snapshot to
be deleted.  These requirements are checked and enforced at the top of
qemuDomainSnapshotDeleteExternal().  They might even be too restrictive for
the case where deletion of children is not requested as in that case,
requiring that the parent of the snapshot to be deleted not be a branching
point in snapshot tree should be enough.

The above limitations do not appear to be too severe under the current state
of matters where a major source of branching snapshot structures,
snapshot-revert, is not even implemented for external snapshots yet.  The only
other known cause of branching in external snapshots thus seems to be if a
user creates branching by manually creating snapshot images and metadata and
applies them using snapshot-create --redefine.  At any rate, this work should
be understood as just a first step to a full support of deleting external
snapshots.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 149 +++--
 1 file changed, 145 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b981745ecf..4d4f3f069f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16707,6 +16707,149 @@ qemuDomainMomentReparentChildren(void *payload,
 }
 
 
+/*
+ * We can't use virDomainMomentFindLeaf() as it takes a
+ * virDomainMomentObjListPtr which we don't have.  It might be a good idea to
+ * move this function to a library of virDomainMomentObj helpers, then
+ * reimplement virDomainMomentFindLeaf() in terms of this function as it only
+ * uses its virDomainMomentObjListPtr parameter to fish a virDomainMomentObjPtr
+ * out of it.  Then it just runs this function's algorithm on it.
+ */
+static virDomainMomentObjPtr
+myDomainMomentFindLeaf(virDomainMomentObjPtr moment)
+{
+if (moment->nchildren != 1)
+return NULL;
+while (moment->nchildren == 1)
+moment = moment->first_child;
+if (moment->nchildren == 0)
+return moment;
+return NULL;
+}
+
+
+static int
+qemuDomainSnapshotDeleteExternal(virDomainObjPtr vm,
+ virQEMUDriverPtr driver,
+ virDomainMomentObjPtr snap,
+ unsigned int flags)
+{
+int ret = -1;
+bool isActive;
+virDomainSnapshotDefPtr snapdef = virDomainSnapshotObjGetDef(snap);
+virDomainMomentObjPtr leaf = snap->nchildren ? 
myDomainMomentFindLeaf(snap) : snap;
+virDomainMomentObjPtr parent = snap->parent;
+virDomainSnapshotDefPtr parentdef = virDomainSnapshotObjGetDef(parent);
+size_t i;
+
+/* This function only works if the chain below 'snap' is linear.  If
+ * there's no unique leaf it means the chain of 'snap's children
+ * branches at some point.  Also, if there *is* a leaf but it's not
+ * the current snapshot, bail out as well. */
+if (leaf == NULL) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   "%s", _("can't delete, snapshot chain branches"));
+goto cleanup;
+}
+if (leaf != virDomainSnapshotGetCurrent(vm->snapshots)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   "%s", _("can't delete, leaf snapshot is not current"));
+goto cleanup;
+}
+
+if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
+ VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) {
+isActive = true;
+} else {
+isActive = snap == virDomainSnapshotGetCurrent(vm->snapshots);
+}
+
+for (i = 0; i < snapdef->ndisks; i++) {
+virDomainSnapshotDiskDefPtr snapdisk = &(snapdef->disks[i]);
+const char *diskName = snapdisk->name;
+const char *basePath = NULL;
+const char *topPath = snapdisk->src->path;
+int blockCommitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE;
+virDomainDiskDefPtr disk;
+
+if (!(disk = qemuDomainDiskByName(vm->def, diskName)))
+goto cleanup;
+
+if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
+basePath = snapdisk->src->path;
+topPath = disk->src->path;
+} else {
+if (parent->nchildren == 1) {
+if (parentdef == NULL

[libvirt PATCH 0/3] qemu: block: basic implementation of deletion of external snapshots

[Pavel Mores]

Deleting external snapshots has been unimplemented so far.  This series aims
to enable limited functionality in that direction, handling just the common
cases for now.  The intention is to subsequently build upon this to eventually
cover the more complex/exotic cases as well.

Please refer to the commit message of the final commit for a detailed
description of features and limitations of this change.

Pavel Mores (3):
  qemu: block: factor implementation out of qemuDomainBlockCommit()
  qemu: block: factor implementation out of qemuDomainBlockJobAbort()
  qemu: block: external snapshot-delete implementation for
straightforward cases

 src/qemu/qemu_driver.c | 295 +
 1 file changed, 241 insertions(+), 54 deletions(-)

-- 
2.24.1

[libvirt PATCH 2/3] qemu: block: factor implementation out of qemuDomainBlockJobAbort()

[Pavel Mores]

As with the previous commit, this should primarily make
qemuDomainBlockJobAbort() callable from within the QEMU driver.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 62 --
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f94636f651..b981745ecf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -163,6 +163,12 @@ qemuDomainBlockCommitImpl(virDomainObjPtr vm,
   unsigned long bandwidth,
   unsigned int flags);
 
+static int
+qemuDomainBlockJobAbortImpl(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+const char *path,
+unsigned int flags);
+
 /* Looks up the domain object from snapshot and unlocks the
  * driver. The returned domain object is locked and ref'd and the
  * caller must call virDomainObjEndAPI() on it. */
@@ -17440,45 +17446,31 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm,
 return ret;
 }
 
-
 static int
-qemuDomainBlockJobAbort(virDomainPtr dom,
-const char *path,
-unsigned int flags)
+qemuDomainBlockJobAbortImpl(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+const char *path,
+unsigned int flags)
 {
-virQEMUDriverPtr driver = dom->conn->privateData;
 virDomainDiskDefPtr disk = NULL;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT);
 bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC);
 g_autoptr(qemuBlockJobData) job = NULL;
-virDomainObjPtr vm;
 qemuDomainObjPrivatePtr priv = NULL;
 bool blockdev = false;
 int ret = -1;
 
-virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
-  VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
-
-if (!(vm = qemuDomainObjFromDomain(dom)))
-return -1;
-
-if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0)
-goto cleanup;
-
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
-goto cleanup;
-
 if (virDomainObjCheckActive(vm) < 0)
-goto endjob;
+return -1;
 
 if (!(disk = qemuDomainDiskByName(vm->def, path)))
-goto endjob;
+return -1;
 
 if (!(job = qemuBlockJobDiskGetJob(disk))) {
 virReportError(VIR_ERR_INVALID_ARG,
_("disk %s does not have an active block job"), 
disk->dst);
-goto endjob;
+return -1;
 }
 
 priv = vm->privateData;
@@ -17549,6 +17541,34 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
  endjob:
 if (job && !async)
 qemuBlockJobSyncEnd(vm, job, QEMU_ASYNC_JOB_NONE);
+
+return ret;
+}
+
+
+static int
+qemuDomainBlockJobAbort(virDomainPtr dom,
+const char *path,
+unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+int ret = -1;
+
+virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC |
+  VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1);
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+return -1;
+
+if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+ret = qemuDomainBlockJobAbortImpl(driver, vm, path, flags);
+
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
-- 
2.24.1

[libvirt PATCH 1/3] qemu: block: factor implementation out of qemuDomainBlockCommit()

[Pavel Mores]

The idea is to make block commit callable from within the QEMU driver.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 84 +++---
 1 file changed, 55 insertions(+), 29 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e9a62684f0..f94636f651 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -154,6 +154,15 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t 
fallback_gid,
 
 static virQEMUDriverPtr qemu_driver;
 
+static int
+qemuDomainBlockCommitImpl(virDomainObjPtr vm,
+  virQEMUDriverPtr driver,
+  const char *path,
+  const char *base,
+  const char *top,
+  unsigned long bandwidth,
+  unsigned int flags);
+
 /* Looks up the domain object from snapshot and unlocks the
  * driver. The returned domain object is locked and ref'd and the
  * caller must call virDomainObjEndAPI() on it. */
@@ -18314,18 +18323,16 @@ qemuDomainBlockPull(virDomainPtr dom, const char 
*path, unsigned long bandwidth,
 return qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags);
 }
 
-
 static int
-qemuDomainBlockCommit(virDomainPtr dom,
-  const char *path,
-  const char *base,
-  const char *top,
-  unsigned long bandwidth,
-  unsigned int flags)
+qemuDomainBlockCommitImpl(virDomainObjPtr vm,
+  virQEMUDriverPtr driver,
+  const char *path,
+  const char *base,
+  const char *top,
+  unsigned long bandwidth,
+  unsigned int flags)
 {
-virQEMUDriverPtr driver = dom->conn->privateData;
-qemuDomainObjPrivatePtr priv;
-virDomainObjPtr vm = NULL;
+qemuDomainObjPrivatePtr priv = vm->privateData;
 const char *device = NULL;
 const char *jobname = NULL;
 int ret = -1;
@@ -18347,25 +18354,6 @@ qemuDomainBlockCommit(virDomainPtr dom,
 bool persistjob = false;
 bool blockdev = false;
 
-virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
-  VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
-  VIR_DOMAIN_BLOCK_COMMIT_RELATIVE |
-  VIR_DOMAIN_BLOCK_COMMIT_DELETE |
-  VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1);
-
-if (!(vm = qemuDomainObjFromDomain(dom)))
-goto cleanup;
-priv = vm->privateData;
-
-if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0)
-goto cleanup;
-
-if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0)
-goto cleanup;
-
-if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
-goto cleanup;
-
 if (virDomainObjCheckActive(vm) < 0)
 goto endjob;
 
@@ -18558,6 +18546,43 @@ qemuDomainBlockCommit(virDomainPtr dom,
 virErrorRestore(_err);
 }
 qemuBlockJobStartupFinalize(vm, job);
+
+return ret;
+}
+
+
+static int
+qemuDomainBlockCommit(virDomainPtr dom,
+  const char *path,
+  const char *base,
+  const char *top,
+  unsigned long bandwidth,
+  unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+int ret = -1;
+
+virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
+  VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
+  VIR_DOMAIN_BLOCK_COMMIT_RELATIVE |
+  VIR_DOMAIN_BLOCK_COMMIT_DELETE |
+  VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1);
+
+if (!(vm = qemuDomainObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+ret = qemuDomainBlockCommitImpl(vm, driver, path, base, top, bandwidth, 
flags);
+
 qemuDomainObjEndJob(driver, vm);
 
  cleanup:
@@ -18565,6 +18590,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
 return ret;
 }
 
+
 static int
 qemuDomainOpenGraphics(virDomainPtr dom,
unsigned int idx,
-- 
2.24.1

Re: [PATCH 6/6] security: Try harder to run transactions

[Pavel Mores]

On Wed, Mar 18, 2020 at 06:32:16PM +0100, Michal Privoznik wrote:
> When a QEMU process dies in the middle of a hotplug, then we fail
> to restore the seclabels on the device. The problem is that if
> the thread doing hotplug locks the domain object first and thus
> blocks the thread that wants to do qemuProcessStop(), the
> seclabel cleanup code will see vm->pid still set and mount
> namespace used and therefore try to enter the namespace
> represented by the PID. But the PID is gone really and thus
> entering will fail and no restore is done. What we can do is to
> try enter the namespace (if requested to do so) but if entering
> fails, fall back to no NS mode.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1814481
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/security/security_dac.c | 16 
>  src/security/security_selinux.c | 16 
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 9046b51004..11fff63bc7 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -640,15 +640,23 @@ virSecurityDACTransactionCommit(virSecurityManagerPtr 
> mgr G_GNUC_UNUSED,
>  
>  list->lock = lock;
>  
> +if (pid != -1) {
> +rc = virProcessRunInMountNamespace(pid,
> +   virSecurityDACTransactionRun,
> +   list);
> +if (rc < 0) {
> +if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR)
> +pid = -1;
> +else
> +goto cleanup;
> +}
> +}
> +
>  if (pid == -1) {
>  if (lock)
>  rc = virProcessRunInFork(virSecurityDACTransactionRun, list);
>  else
>  rc = virSecurityDACTransactionRun(pid, list);
> -} else {
> -rc = virProcessRunInMountNamespace(pid,
> -   virSecurityDACTransactionRun,
> -   list);
>  }
>  
>  if (rc < 0)
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index c94f31727c..8aeb6e45a5 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1163,15 +1163,23 @@ 
> virSecuritySELinuxTransactionCommit(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>  
>  list->lock = lock;
>  
> +if (pid != -1) {
> +rc = virProcessRunInMountNamespace(pid,
> +   virSecuritySELinuxTransactionRun,
> +   list);
> +if (rc < 0) {
> +if (virGetLastErrorCode() == VIR_ERR_SYSTEM_ERROR)
> +pid = -1;
> +else
> +goto cleanup;
> +}
> +}
> +
>  if (pid == -1) {
>  if (lock)
>  rc = virProcessRunInFork(virSecuritySELinuxTransactionRun, list);
>  else
>  rc = virSecuritySELinuxTransactionRun(pid, list);
> -} else {
> -rc = virProcessRunInMountNamespace(pid,
> -   virSecuritySELinuxTransactionRun,
> -   list);
>  }
>  
>  if (rc < 0)
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 5/6] virprocess: Passthru error from virProcessRunInForkHelper

[Pavel Mores]

  }
>  }
>  }
>  }
> diff --git a/tests/commandtest.c b/tests/commandtest.c
> index a64aa9ad33..f4a2c67c05 100644
> --- a/tests/commandtest.c
> +++ b/tests/commandtest.c
> @@ -1257,6 +1257,48 @@ static int test27(const void *unused G_GNUC_UNUSED)
>  }
>  
>  
> +static int
> +test28Callback(pid_t pid G_GNUC_UNUSED,
> +   void *opaque G_GNUC_UNUSED)
> +{
> +virReportSystemError(ENODATA, "%s", "some error message");
> +return -1;
> +}
> +
> +
> +static int
> +test28(const void *unused G_GNUC_UNUSED)
> +{
> +/* Not strictly a virCommand test, but this is the easiest place
> + * to test this lower-level interface. */
> +virErrorPtr err;
> +
> +if (virProcessRunInFork(test28Callback, NULL) != -1) {
> +fprintf(stderr, "virProcessRunInFork did not fail\n");
> +return -1;
> +}
> +
> +if (!(err = virGetLastError())) {
> +fprintf(stderr, "Expected error but got nothing\n");
> +return -1;
> +}
> +
> +if (!(err->code == VIR_ERR_SYSTEM_ERROR &&
> +  err->domain == 0 &&
> +  STREQ(err->message, "some error message: No data available") &&
> +  err->level == VIR_ERR_ERROR &&
> +  STREQ(err->str1, "%s") &&
> +  STREQ(err->str2, "some error message: No data available") &&
> +  err->int1 == ENODATA &&
> +  err->int2 == -1)) {
> +fprintf(stderr, "Unexpected error object\n");
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +
>  static int
>  mymain(void)
>  {
> @@ -1354,6 +1396,7 @@ mymain(void)
>  DO_TEST(test25);
>  DO_TEST(test26);
>  DO_TEST(test27);
> +DO_TEST(test28);
>  
>  return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>  }
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 2/6] qemuDomainBuildNamespace: Try harder to remove temp directories

[Pavel Mores]

On Wed, Mar 18, 2020 at 06:32:12PM +0100, Michal Privoznik wrote:
> If building namespace fails somewhere in the middle (that is some
> files exists under devMountsSavePath[i]), then plain rmdir() is
> not enough to remove dir. Umount the temp location and use
> virFileDeleteTree() to remove the directory.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 48bf5ae559..2724607311 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -15311,9 +15311,12 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>  ret = 0;
>   cleanup:
>  for (i = 0; i < ndevMountsPath; i++) {
> +#if defined(__linux__)
> +umount(devMountsSavePath[i]);
> +#endif /* defined(__linux__) */
>  /* The path can be either a regular file or a dir. */
>  if (virFileIsDir(devMountsSavePath[i]))
> -rmdir(devMountsSavePath[i]);
> +virFileDeleteTree(devMountsSavePath[i]);
>  else
>      unlink(devMountsSavePath[i]);
>  }
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 1/6] qemuDomainCreateDeviceRecursive: Report error if mkdir() fails

[Pavel Mores]

On Wed, Mar 18, 2020 at 06:32:11PM +0100, Michal Privoznik wrote:
> The virFileMakePathWithMode() which is our recursive version of
> mkdir() fails, it simply just returns a negative value with errno
> set. No error is reported (as compared to virFileTouch() for
> instance).
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 0e2252f6cf..48bf5ae559 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -14643,8 +14643,12 @@ qemuDomainCreateDeviceRecursive(const char *device,
>   * proper owner and mode. Bind mount only after that. */
>  } else if (isDir) {
>  if (create &&
> -virFileMakePathWithMode(devicePath, sb.st_mode) < 0)
> +virFileMakePathWithMode(devicePath, sb.st_mode) < 0) {
> +virReportSystemError(errno,
> + _("Unable to make dir %s"),
> + devicePath);
>  goto cleanup;
> +}
>  } else {
>  virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("unsupported device type %s 0%o"),
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 4/6] virfile: Handle directories in virFileBindMountDevice()

[Pavel Mores]

On Wed, Mar 18, 2020 at 06:32:14PM +0100, Michal Privoznik wrote:
> The @src is not always a file. It may also be a directory (for
> instance qemuDomainCreateDeviceRecursive() assumes that) - even
> though it doesn't happen usually. Anyway, mount() can mount only
> a dir onto a dir and a file onto a file.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/virfile.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 0f31554323..9c175b041f 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3743,8 +3743,17 @@ int
>  virFileBindMountDevice(const char *src,
> const char *dst)
>  {
> -if (virFileTouch(dst, 0666) < 0)
> -return -1;
> +if (!virFileExists(dst)) {
> +if (virFileIsDir(src)) {
> +if (virFileMakePath(dst)) {

Personally, I'd consider

if (virFileMakePath(dst) < 0)

clearer here for a reader who's not familiar with the function and what exactly
it returns.

Reviewed-by: Pavel Mores 

> +virReportSystemError(errno, _("Unable to make dir %s"), dst);
> +return -1;
> +}
> +} else {
> +if (virFileTouch(dst, 0666) < 0)
> +return -1;
> +}
> +}
>  
>  if (mount(src, dst, "none", MS_BIND, NULL) < 0) {
>  virReportSystemError(errno, _("Failed to bind %s on to %s"), src,
> -- 
> 2.24.1
> 

Re: [PATCH 3/6] qemuDomainBuildNamespace: Make @devPath const

[Pavel Mores]

On Wed, Mar 18, 2020 at 06:32:13PM +0100, Michal Privoznik wrote:
> The @devPath variable is not modifiable. It merely just points to
> string containing path where private devtmpfs is being
> constructed. Make it const so it doesn't look weird that it's not
> freed.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2724607311..a4c07fffcb 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -15176,7 +15176,7 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>   virDomainObjPtr vm)
>  {
>  struct qemuDomainCreateDeviceData data;
> -char *devPath = NULL;
> +const char *devPath = NULL;
>  char **devMountsPath = NULL, **devMountsSavePath = NULL;
>  size_t ndevMountsPath = 0, i;
>  int ret = -1;
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 05/13] qemu: block: Implement helpers for dealing with bitmaps during block commit

[Pavel Mores]

On Wed, Mar 04, 2020 at 06:26:33PM +0100, Peter Krempa wrote:
> qemuBlockBitmapsHandleCommitStart prepares for disabling the bitmaps in
> the 'base' of the commit job so that the bitmaps are not dirtied by the
> commit job. This needs to be done prior to start of the commit job.
> 
> qemuBlockBitmapsHandleCommitFinish then calculates the necessary merges
> that agregate all the bitmaps between the commited images and write them
> into the base bitmap.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_block.c | 217 ++
>  src/qemu/qemu_block.h |  14 +++
>  2 files changed, 231 insertions(+)
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 11df8eedd0..2467315563 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -2962,6 +2962,223 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr 
> src,
>  }
> 
> 
> +/**
> + * @topsrc: virStorageSource representing 'top' of the job
> + * @basesrc: virStorageSource representing 'base' of the job
> + * @blockNamedNodeData: hash table containing data about bitmaps
> + * @actions: filled with arguments for a 'transaction' command
> + * @disabledBitmapsBase: filled with a list of bitmap names which must be 
> disabled
> + *
> + * Prepares data for correctly hanlding bitmaps during the start of a commit
> + * job. The bitmaps in the 'base' image must be disabled, so that the writes
> + * done by the blockjob don't dirty the enabled bitmaps.
> + *
> + * @actions and @disabledBitmapsBase are untouched if no bitmaps need
> + * to be disabled.
> + */
> +int
> +qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr top,

According to the documentation above and by symmetry with other changes in this
commit, shouldn't this parameter be actually called 'topsrc'?

> +  virStorageSourcePtr basesrc,
> +  virHashTablePtr blockNamedNodeData,
> +  virJSONValuePtr *actions,
> +  char ***disabledBitmapsBase)
> +{
> +g_autoptr(virJSONValue) act = virJSONValueNewArray();
> +VIR_AUTOSTRINGLIST bitmaplist = NULL;
> +size_t curbitmapstr = 0;
> +qemuBlockNamedNodeDataPtr entry;
> +bool disable_bitmaps = true;
> +size_t i;
> +
> +if (!(entry = virHashLookup(blockNamedNodeData, basesrc->nodeformat)))
> +return 0;
> +
> +bitmaplist = g_new0(char *, entry->nbitmaps);
> +
> +for (i = 0; i < entry->nbitmaps; i++) {
> +qemuBlockNamedNodeDataBitmapPtr bitmap = entry->bitmaps[i];
> +
> +if (!bitmap->recording || bitmap->inconsistent ||
> +!qemuBlockBitmapChainIsValid(top, bitmap->name, 
> blockNamedNodeData))
> +continue;
> +
> +disable_bitmaps = true;
> +
> +if (qemuMonitorTransactionBitmapDisable(act, basesrc->nodeformat,
> +bitmap->name) < 0)
> +return -1;
> +
> +bitmaplist[curbitmapstr++] = g_strdup(bitmap->name);
> +}
> +
> +if (disable_bitmaps) {

Can 'disable_bitmaps' actually be 'false' at this point?  Perhaps it should be
initialised to 'false' at the top of this function?

> +*actions = g_steal_pointer();
> +*disabledBitmapsBase = g_steal_pointer();
> +}
> +
> +return 0;
> +}
> +
> +
> +struct qemuBlockBitmapsHandleCommitData {
> +bool skip;
> +bool create;
> +bool enable;
> +const char *basenode;
> +virJSONValuePtr merge;
> +unsigned long long granularity;
> +bool persistent;
> +};
> +
> +
> +static void
> +qemuBlockBitmapsHandleCommitDataFree(void *opaque)
> +{
> +struct qemuBlockBitmapsHandleCommitData *data = opaque;
> +
> +virJSONValueFree(data->merge);
> +g_free(data);
> +}
> +
> +
> +static int
> +qemuBlockBitmapsHandleCommitFinishIterate(void *payload,
> +  const void *entryname,
> +  void *opaque)
> +{
> +struct qemuBlockBitmapsHandleCommitData *data = payload;
> +const char *bitmapname = entryname;
> +virJSONValuePtr actions = opaque;
> +
> +if (data->skip)
> +return 0;
> +
> +if (data->create) {
> +if (qemuMonitorTransactionBitmapAdd(actions, data->basenode, 
> bitmapname,
> +data->persistent, !data-> enable,
> +data->granularity) < 0)
> +return -1;
> +} else {
> +if (data->enable &&
> +qemuMonitorTransactionBitmapEnable(actions, data->basenode, 
> bitmapname) < 0)
> +return -1;
> +}
> +
> +if (data->merge &&
> +qemuMonitorTransactionBitmapMerge(actions, data->basenode, 
> bitmapname,
> +  >merge) < 0)
> +return -1;
> +
> +return 0;
> +}
> +
> +
> +/**
> + * @topsrc: virStorageSource representing 'top' of the job
> + * 

Re: [PATCH 03/13] qemu: blockjob: Store list of bitmaps disabled prior to commit

[Pavel Mores]

On Wed, Mar 04, 2020 at 06:26:31PM +0100, Peter Krempa wrote:
> Starting a commit job will require disabling bitmaps in the base image
> so that they are not dirtied by the commit job. We need to store a list
> of the bitmaps so that we can later re-enable them.
> 
> Add a field and status XML handling code as well as a test.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_blockjob.h  |  2 ++
>  src/qemu/qemu_domain.c| 26 +++
>  .../blockjob-blockdev-in.xml  |  4 +++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
> index 72c7fa053e..e2e28ca4d3 100644
> --- a/src/qemu/qemu_blockjob.h
> +++ b/src/qemu/qemu_blockjob.h
> @@ -88,6 +88,8 @@ struct _qemuBlockJobCommitData {
>  virStorageSourcePtr top;
>  virStorageSourcePtr base;
>  bool deleteCommittedImages;
> +char **disabledBitmapsBase; /* a NULL-terminated list of bitmap names 
> which
> +   were disabled in @base for the commit job 
> */
>  };

Is it guaranteed that the new member 'disabledBitmapsBase' is properly
initialised?  How about something along the lines of


diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 2e53821d43..f9fc83430e 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -312,6 +312,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
 job->data.commit.top = top;
 job->data.commit.base = base;
 job->data.commit.deleteCommittedImages = delete_imgs;
+job->data.commit.disabledBitmapsBase = NULL;
 job->jobflags = jobflags;
 
 if (qemuBlockJobRegister(job, vm, disk, true) < 0)


Even if explicit initialisation is somehow not needed here, I'd consider that
fact worth a line of explanation in the commit message.


> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 0a478d2080..c5f4b0ae7f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2534,6 +2534,9 @@ static void
>  qemuDomainPrivateBlockJobFormatCommit(qemuBlockJobDataPtr job,
>virBufferPtr buf)
>  {
> +g_auto(virBuffer) disabledBitmapsBuf = VIR_BUFFER_INIT_CHILD(buf);
> +char **bitmaps = job->data.commit.disabledBitmapsBase;
> +
>  if (job->data.commit.base)
>  virBufferAsprintf(buf, "\n", 
> job->data.commit.base->nodeformat);
> 
> @@ -2545,6 +2548,11 @@ 
> qemuDomainPrivateBlockJobFormatCommit(qemuBlockJobDataPtr job,
> 
>  if (job->data.commit.deleteCommittedImages)
>  virBufferAddLit(buf, "\n");
> +
> +while (bitmaps && *bitmaps)
> +virBufferEscapeString(, "\n", 
> *(bitmaps++));
> +
> +virXMLFormatElement(buf, "disabledBaseBitmaps", NULL, 
> );
>  }
> 
> 
> @@ -3157,6 +3165,9 @@ static int
>  qemuDomainObjPrivateXMLParseBlockjobDataCommit(qemuBlockJobDataPtr job,
> xmlXPathContextPtr ctxt)
>  {
> +g_autofree xmlNodePtr *nodes = NULL;
> +ssize_t nnodes;
> +
>  if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT) {
>  qemuDomainObjPrivateXMLParseBlockjobNodename(job,
>   
> "string(./topparent/@node)",
> @@ -3183,6 +3194,21 @@ 
> qemuDomainObjPrivateXMLParseBlockjobDataCommit(qemuBlockJobDataPtr job,
>  !job->data.commit.base)
>  return -1;
> 
> +if ((nnodes = virXPathNodeSet("./disabledBaseBitmaps/bitmap", ctxt, 
> )) > 0) {
> +size_t i;
> +
> +job->data.commit.disabledBitmapsBase = g_new0(char *, nnodes + 1);
> +
> +for (i = 0; i < nnodes; i++) {
> +char *tmp;
> +
> +if (!(tmp = virXMLPropString(nodes[i], "name")))
> +return -1;
> +
> +job->data.commit.disabledBitmapsBase[i] = g_steal_pointer();
> +}
> +}
> +
>  return 0;
>  }
> 
> diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml 
> b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
> index ca6d110179..cc17a17ff4 100644
> --- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
> +++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
> @@ -243,6 +243,10 @@
>
>
>
> +  
> +
> +
> +  
>  
>   state='running'>
>
> -- 
> 2.24.1
> 

Re: [PATCH 01/13] qemu: domain: Extract formatting of 'commit' blockjob data into a function

[Pavel Mores]

On Wed, Mar 04, 2020 at 06:26:29PM +0100, Peter Krempa wrote:
> I'll be adding more fields to care about so splitting the code out will
> be better long-term.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_domain.c | 27 +++
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 33c2158eb5..71d0a400cc 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2530,6 +2530,24 @@ 
> qemuDomainObjPrivateXMLFormatBlockjobFormatSource(virBufferPtr buf,
>  }
> 
> 
> +static void
> +qemuDomainPrivateBlockJobFormatCommit(qemuBlockJobDataPtr job,
> +  virBufferPtr buf)
> +{
> +if (job->data.commit.base)
> +virBufferAsprintf(buf, "\n", 
> job->data.commit.base->nodeformat);
> +
> +if (job->data.commit.top)
> +virBufferAsprintf(buf, "\n", 
> job->data.commit.top->nodeformat);
> +
> +if (job->data.commit.topparent)
> +virBufferAsprintf(buf, "\n", 
> job->data.commit.topparent->nodeformat);
> +
> +if (job->data.commit.deleteCommittedImages)
> +virBufferAddLit(buf, "\n");
> +}
> +
> +
>  static int
>  qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload,
>const void *name G_GNUC_UNUSED,
> @@ -2589,14 +2607,7 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void 
> *payload,
> 
>  case QEMU_BLOCKJOB_TYPE_COMMIT:
>  case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
> -if (job->data.commit.base)
> -virBufferAsprintf(, "\n", 
> job->data.commit.base->nodeformat);
> -if (job->data.commit.top)
> -virBufferAsprintf(, "\n", 
> job->data.commit.top->nodeformat);
> -if (job->data.commit.topparent)
> -virBufferAsprintf(, "\n", 
> job->data.commit.topparent->nodeformat);
> -if (job->data.commit.deleteCommittedImages)
> -virBufferAddLit(, "\n");
> +qemuDomainPrivateBlockJobFormatCommit(job, );
>  break;
> 
>  case QEMU_BLOCKJOB_TYPE_CREATE:
> -- 
> 2.24.1
> 

Reviewed-by: Pavel Mores 

Re: [PATCH 02/13] qemu: domain: Extract parsing of 'commit' blockjob data into a function

[Pavel Mores]

On Wed, Mar 04, 2020 at 06:26:30PM +0100, Peter Krempa wrote:
> I'll be adding more fields to care about so splitting the code out will
> be better long-term.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_domain.c | 57 ++
>  1 file changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 71d0a400cc..0a478d2080 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3153,6 +3153,40 @@ 
> qemuDomainObjPrivateXMLParseBlockjobNodename(qemuBlockJobDataPtr job,
>  }
> 
> 
> +static int
> +qemuDomainObjPrivateXMLParseBlockjobDataCommit(qemuBlockJobDataPtr job,
> +   xmlXPathContextPtr ctxt)
> +{
> +if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT) {
> +qemuDomainObjPrivateXMLParseBlockjobNodename(job,
> + 
> "string(./topparent/@node)",
> + 
> >data.commit.topparent,
> + ctxt);
> +
> +if (!job->data.commit.topparent)
> +return -1;
> +}
> +
> +qemuDomainObjPrivateXMLParseBlockjobNodename(job,
> + "string(./top/@node)",
> + >data.commit.top,
> + ctxt);
> +qemuDomainObjPrivateXMLParseBlockjobNodename(job,
> + "string(./base/@node)",
> + >data.commit.base,
> + ctxt);
> +
> +if (virXPathNode("./deleteCommittedImages", ctxt))
> +job->data.commit.deleteCommittedImages = true;
> +
> +if (!job->data.commit.top ||
> +!job->data.commit.base)
> +return -1;
> +
> +return 0;
> +}
> +
> +
>  static void
>  qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job,
>   xmlXPathContextPtr ctxt,
> @@ -3172,29 +3206,10 @@ 
> qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job,
>  break;
> 
>  case QEMU_BLOCKJOB_TYPE_COMMIT:
> -qemuDomainObjPrivateXMLParseBlockjobNodename(job,
> - 
> "string(./topparent/@node)",
> - 
> >data.commit.topparent,
> - ctxt);
> -
> -if (!job->data.commit.topparent)
> -goto broken;
> -
> -G_GNUC_FALLTHROUGH;
>  case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
> -qemuDomainObjPrivateXMLParseBlockjobNodename(job,
> - 
> "string(./top/@node)",
> - 
> >data.commit.top,
> - ctxt);
> -qemuDomainObjPrivateXMLParseBlockjobNodename(job,
> - 
> "string(./base/@node)",
> - 
> >data.commit.base,
> - ctxt);
> -if (virXPathNode("./deleteCommittedImages", ctxt))
> -    job->data.commit.deleteCommittedImages = true;
> -if (!job->data.commit.top ||
> -!job->data.commit.base)
> +if (qemuDomainObjPrivateXMLParseBlockjobDataCommit(job, ctxt) < 
> 0)
>  goto broken;
> +
>  break;
> 
>  case QEMU_BLOCKJOB_TYPE_CREATE:
> -- 
> 2.24.1

Reviewed-by: Pavel Mores 

Re: [libvirt PATCH 3/6] qemu: block: blockpull param 'top' support in virsh proper

[Pavel Mores]

On Wed, Mar 04, 2020 at 03:13:41PM +0100, Peter Krempa wrote:
> On Wed, Mar 04, 2020 at 15:01:07 +0100, Pavel Mores wrote:
> > On Wed, Mar 04, 2020 at 02:53:53PM +0100, Peter Krempa wrote:
> > > On Wed, Mar 04, 2020 at 14:41:09 +0100, Pavel Mores wrote:
> > > > On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
> > > > > On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
> > > > > > A new command-line option --top was added to virsh's blockpull 
> > > > > > command.
> > > > > > Similar to how --base is handled, in presence of --top the 
> > > > > > operation is
> > > > > > implemented internally as a rebase.  To that end, a corresponding 
> > > > > > new 'top'
> > > > > > parameter was added to virDomainBlockRebase().
> > > > > > 
> > > > > > Signed-off-by: Pavel Mores 
> > > > > > ---
> > > > > >  include/libvirt/libvirt-domain.h |  4 ++--
> > > > > >  src/libvirt-domain.c |  5 +++--
> > > > > >  tools/virsh-domain.c | 14 +++---
> > > > > >  3 files changed, 16 insertions(+), 7 deletions(-)
> 
> [...]
> 
> > > This is obviously a lot of work, thus we need to decide whether adding
> > > an old-school API is worth it in the inerim. Are there any real users
> > > who would benefit from the new pull semantics? blockpull is around for a
> > > long time already, but it seems that commit is favoured.
> > > 
> > > If there is no real demand though I'd probably prefer if we don't add
> > > any more block job APIs any more.
> > 
> > I'm not aware of any real demand for this, however as I stated in the cover
> > letter I believe I need full blockpull to deal with the bug I'm actually
> > working on, which is full support for external snapshots in snapshot-delete.
> 
> Deleting/reverting external snapshots needs to be done internally under
> the hood of virDomainRevertToSnapshot/virDomainSnapshotDelete so if you
> require use of the 'block-stream' command to an intermediate layer you
> don't actually need to expose it via virDomainBlockPull/Rebase to take
> advantage of it.

That's true.  I think this basically means I should drop patches 3 a 4 from
this series (I'll keep them locally as they give me a reasonably easy way to
test my changes), right?

> For reversion of external snapshots you'll probably need a new API
> anyways as you'll need to be able to specify a new set of disk images to
> hold the writes.

Okay, I'll deal with that in due time, I guess when I'm back to working on
snapshot-delete.

pvl

Re: [libvirt PATCH 3/6] qemu: block: blockpull param 'top' support in virsh proper

[Pavel Mores]

On Wed, Mar 04, 2020 at 02:53:53PM +0100, Peter Krempa wrote:
> On Wed, Mar 04, 2020 at 14:41:09 +0100, Pavel Mores wrote:
> > On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
> > > On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
> > > > A new command-line option --top was added to virsh's blockpull command.
> > > > Similar to how --base is handled, in presence of --top the operation is
> > > > implemented internally as a rebase.  To that end, a corresponding new 
> > > > 'top'
> > > > parameter was added to virDomainBlockRebase().
> > > > 
> > > > Signed-off-by: Pavel Mores 
> > > > ---
> > > >  include/libvirt/libvirt-domain.h |  4 ++--
> > > >  src/libvirt-domain.c |  5 +++--
> > > >  tools/virsh-domain.c | 14 +++---
> > > >  3 files changed, 16 insertions(+), 7 deletions(-)
> 
> [...]
> 
> > > > @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char 
> > > > *disk,
> > > >   */
> > > >  int
> > > >  virDomainBlockRebase(virDomainPtr dom, const char *disk,
> > > > - const char *base, unsigned long bandwidth,
> > > > - unsigned int flags)
> > > > + const char *base, const char *top,
> > > > + unsigned long bandwidth, unsigned int flags)
> > > >  {
> > > >  virConnectPtr conn;
> > > 
> > > So this is one thing we can't do. This modifies the public API of
> > > libvirt and would cause software which is already compiled to pass wrong
> > > arguments to this function.
> > > 
> > > If the semantics can't be mapped to existing arguments of this function
> > > we'll need to add a new function in the first place.
> > 
> > Yes.  Seeing as the function takes just a bunch of primitive data types and 
> > a
> > virDomainPtr, I'm afraid I don't see much room for not changing the 
> > signature,
> > apart from arguably dubious tricks like stuffing both base and top to the
> > existing 'base' parameter and parsing the individual values out of it in the
> > function body.
> > 
> > Any convention or suggestion to help pick a good name for the new function?
> 
> If we are going to implement new APIs for blockjobs I'd prefer we take a
> step back and re-design the (block)-job APIs rather than add yet another
> oldschool one.
> 
> Similarly to our criticisms of qemu's apis which didn't preserve the job
> state after the job finished, which is now fixed I've heard exactly the
> same requests from at least oVirt developers requesting we do the same
> thing.
> 
> The new API(s) should thus return a job object (virDomainJob?) similarly
> to how the domain lookup APIs return them. This new object then can be
> used to refer to the job even after it has finished. Similarly to qemu
> we should also introduce provisions to auto-dismiss the job if the
> management APP doesn't wish to have to deal with it (and that probably
> should be default behaviour given how we approached it previously).
> 
> This will then obviously require support APIs for looking up/listing
> jobs, getting the state or configuration etc.
> 
> This is obviously a lot of work, thus we need to decide whether adding
> an old-school API is worth it in the inerim. Are there any real users
> who would benefit from the new pull semantics? blockpull is around for a
> long time already, but it seems that commit is favoured.
> 
> If there is no real demand though I'd probably prefer if we don't add
> any more block job APIs any more.

I'm not aware of any real demand for this, however as I stated in the cover
letter I believe I need full blockpull to deal with the bug I'm actually
working on, which is full support for external snapshots in snapshot-delete.

Considering what you said, the "dubious trick" I mentioned in the other branch
of this thread might be a bearable interim solution...

pvl

Re: [libvirt PATCH 3/6] qemu: block: blockpull param 'top' support in virsh proper

[Pavel Mores]

On Wed, Mar 04, 2020 at 02:41:09PM +0100, Pavel Mores wrote:
> On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
> > On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
> > > A new command-line option --top was added to virsh's blockpull command.
> > > Similar to how --base is handled, in presence of --top the operation is
> > > implemented internally as a rebase.  To that end, a corresponding new 
> > > 'top'
> > > parameter was added to virDomainBlockRebase().
> > > 
> > > Signed-off-by: Pavel Mores 
> > > ---
> > >  include/libvirt/libvirt-domain.h |  4 ++--
> > >  src/libvirt-domain.c |  5 +++--
> > >  tools/virsh-domain.c | 14 +++---
> > >  3 files changed, 16 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/include/libvirt/libvirt-domain.h 
> > > b/include/libvirt/libvirt-domain.h
> > > index b440818ec2..069d7f98ec 100644
> > > --- a/include/libvirt/libvirt-domain.h
> > > +++ b/include/libvirt/libvirt-domain.h
> > > @@ -2560,8 +2560,8 @@ typedef enum {
> > >  } virDomainBlockRebaseFlags;
> > >  
> > >  int virDomainBlockRebase(virDomainPtr dom, const char *disk,
> > > - const char *base, unsigned long bandwidth,
> > > - unsigned int flags);
> > > + const char *base, const char *top,
> > > + unsigned long bandwidth, unsigned int flags);
> > >  
> > >  /**
> > >   * virDomainBlockCopyFlags:
> > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > > index 65813b68cc..1f9d1b5b84 100644
> > > --- a/src/libvirt-domain.c
> > > +++ b/src/libvirt-domain.c
> > > @@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char 
> > > *disk,
> > >   * @disk: path to the block device, or device shorthand
> > >   * @base: path to backing file to keep, or device shorthand,
> > >   *or NULL for no backing file
> > > + * @top: path to top file, or device shorthand, or NULL for no top
> > >   * @bandwidth: (optional) specify bandwidth limit; flags determine the 
> > > unit
> > >   * @flags: bitwise-OR of virDomainBlockRebaseFlags
> > >   *
> > > @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char 
> > > *disk,
> > >   */
> > >  int
> > >  virDomainBlockRebase(virDomainPtr dom, const char *disk,
> > > - const char *base, unsigned long bandwidth,
> > > - unsigned int flags)
> > > + const char *base, const char *top,
> > > + unsigned long bandwidth, unsigned int flags)
> > >  {
> > >  virConnectPtr conn;
> > 
> > So this is one thing we can't do. This modifies the public API of
> > libvirt and would cause software which is already compiled to pass wrong
> > arguments to this function.
> > 
> > If the semantics can't be mapped to existing arguments of this function
> > we'll need to add a new function in the first place.
> 
> Yes.  Seeing as the function takes just a bunch of primitive data types and a
> virDomainPtr, I'm afraid I don't see much room for not changing the signature,
> apart from arguably dubious tricks like stuffing both base and top to the
> existing 'base' parameter and parsing the individual values out of it in the
> function body.

Actually, on second thought, it might not be as dubious as I first thought.
Certainly not as clean as just adding a parameter in general but depending on
what the cost of adding a new API would be, reusing the existing 'base' param
might be workable.

Also, how about the RPC change, is that acceptable?

Thanks,

pvl

Re: [libvirt PATCH 3/6] qemu: block: blockpull param 'top' support in virsh proper

[Pavel Mores]

On Wed, Mar 04, 2020 at 01:00:59PM +0100, Peter Krempa wrote:
> On Wed, Mar 04, 2020 at 11:12:37 +0100, Pavel Mores wrote:
> > A new command-line option --top was added to virsh's blockpull command.
> > Similar to how --base is handled, in presence of --top the operation is
> > implemented internally as a rebase.  To that end, a corresponding new 'top'
> > parameter was added to virDomainBlockRebase().
> > 
> > Signed-off-by: Pavel Mores 
> > ---
> >  include/libvirt/libvirt-domain.h |  4 ++--
> >  src/libvirt-domain.c |  5 +++--
> >  tools/virsh-domain.c | 14 +++---
> >  3 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/libvirt/libvirt-domain.h 
> > b/include/libvirt/libvirt-domain.h
> > index b440818ec2..069d7f98ec 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -2560,8 +2560,8 @@ typedef enum {
> >  } virDomainBlockRebaseFlags;
> >  
> >  int virDomainBlockRebase(virDomainPtr dom, const char *disk,
> > - const char *base, unsigned long bandwidth,
> > - unsigned int flags);
> > + const char *base, const char *top,
> > + unsigned long bandwidth, unsigned int flags);
> >  
> >  /**
> >   * virDomainBlockCopyFlags:
> > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > index 65813b68cc..1f9d1b5b84 100644
> > --- a/src/libvirt-domain.c
> > +++ b/src/libvirt-domain.c
> > @@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char 
> > *disk,
> >   * @disk: path to the block device, or device shorthand
> >   * @base: path to backing file to keep, or device shorthand,
> >   *or NULL for no backing file
> > + * @top: path to top file, or device shorthand, or NULL for no top
> >   * @bandwidth: (optional) specify bandwidth limit; flags determine the unit
> >   * @flags: bitwise-OR of virDomainBlockRebaseFlags
> >   *
> > @@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char 
> > *disk,
> >   */
> >  int
> >  virDomainBlockRebase(virDomainPtr dom, const char *disk,
> > - const char *base, unsigned long bandwidth,
> > - unsigned int flags)
> > + const char *base, const char *top,
> > + unsigned long bandwidth, unsigned int flags)
> >  {
> >  virConnectPtr conn;
> 
> So this is one thing we can't do. This modifies the public API of
> libvirt and would cause software which is already compiled to pass wrong
> arguments to this function.
> 
> If the semantics can't be mapped to existing arguments of this function
> we'll need to add a new function in the first place.

Yes.  Seeing as the function takes just a bunch of primitive data types and a
virDomainPtr, I'm afraid I don't see much room for not changing the signature,
apart from arguably dubious tricks like stuffing both base and top to the
existing 'base' parameter and parsing the individual values out of it in the
function body.

Any convention or suggestion to help pick a good name for the new function?

Thanks,

pvl

[libvirt PATCH 4/6] qemu: block: blockpull param 'top' support in driver and RPC

[Pavel Mores]

Rebase implementation in QEMU driver extended with a new 'top' parameter.
Support for 'top' added to RPC's remote_domain_block_rebase_args structure.

Signed-off-by: Pavel Mores 
---
 src/driver-hypervisor.h  | 1 +
 src/libvirt-domain.c | 2 +-
 src/qemu/qemu_driver.c   | 3 ++-
 src/remote/remote_protocol.x | 1 +
 src/remote_protocol-structs  | 1 +
 5 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index bce023017d..51a25c41e3 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1053,6 +1053,7 @@ typedef int
 (*virDrvDomainBlockRebase)(virDomainPtr dom,
const char *path,
const char *base,
+   const char *top,
unsigned long bandwidth,
unsigned int flags);
 
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 1f9d1b5b84..33e307cb26 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10287,7 +10287,7 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk,
 
 if (conn->driver->domainBlockRebase) {
 int ret;
-ret = conn->driver->domainBlockRebase(dom, disk, base, bandwidth,
+ret = conn->driver->domainBlockRebase(dom, disk, base, top, bandwidth,
   flags);
 if (ret < 0)
 goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7970c913f3..b3f97bf593 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18143,7 +18143,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 
 static int
 qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
-  unsigned long bandwidth, unsigned int flags)
+  const char *top, unsigned long bandwidth,
+  unsigned int flags)
 {
 virDomainObjPtr vm;
 int ret = -1;
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index d4393680e9..5f5a921057 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -1414,6 +1414,7 @@ struct remote_domain_block_rebase_args {
 remote_nonnull_domain dom;
 remote_nonnull_string path;
 remote_string base;
+remote_string top;
 unsigned hyper bandwidth;
 unsigned int flags;
 };
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index bae0f0b545..b317ec9102 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -992,6 +992,7 @@ struct remote_domain_block_rebase_args {
 remote_nonnull_domain  dom;
 remote_nonnull_string  path;
 remote_string  base;
+remote_string  top;
 uint64_t   bandwidth;
 u_int  flags;
 };
-- 
2.24.1

[libvirt PATCH 1/6] qemu: block: add 'top' parameter to qemuDomainBlockPullCommon()

[Pavel Mores]

qemuDomainBlockPullCommon() is one of the functions in block pull
implementation that have to support 'top' to make block pull as
a whole to support it.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e9a62684f0..81cca360e0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17311,6 +17311,7 @@ static int
 qemuDomainBlockPullCommon(virDomainObjPtr vm,
   const char *path,
   const char *base,
+  const char *top,
   unsigned long bandwidth,
   unsigned int flags)
 {
@@ -17322,6 +17323,9 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm,
 virStorageSourcePtr baseSource = NULL;
 unsigned int baseIndex = 0;
 g_autofree char *basePath = NULL;
+virStorageSourcePtr topSource = NULL;
+unsigned int topIndex = 0;
+g_autofree char *topPath = NULL;
 g_autofree char *backingPath = NULL;
 unsigned long long speed = bandwidth;
 qemuBlockJobDataPtr job = NULL;
@@ -17355,6 +17359,12 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm,
   base, baseIndex, NULL
 goto endjob;
 
+if (top &&
+(virStorageFileParseChainIndex(disk->dst, top, ) < 0 ||
+ !(topSource = virStorageFileChainLookup(disk->src, disk->src,
+  top, topIndex, NULL
+goto endjob;
+
 if (baseSource) {
 if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) {
 if (!virQEMUCapsGet(priv->qemuCaps, 
QEMU_CAPS_CHANGE_BACKING_FILE)) {
@@ -17388,7 +17398,7 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm,
 speed <<= 20;
 }
 
-if (!(job = qemuBlockJobDiskNewPull(vm, disk, baseSource, flags)))
+if (!(job = qemuBlockJobDiskNewPull(vm, disk, baseSource, /*topSource, 
*/flags)))
 goto endjob;
 
 if (blockdev) {
@@ -18160,7 +18170,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char 
*path, const char *base,
 /* For normal rebase (enhanced blockpull), the common code handles
  * everything, including vm cleanup. */
 if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY))
-return qemuDomainBlockPullCommon(vm, path, base, bandwidth, flags);
+return qemuDomainBlockPullCommon(vm, path, base, NULL, bandwidth, 
flags);
 
 /* If we got here, we are doing a block copy rebase. */
 if (!(dest = virStorageSourceNew()))
@@ -18311,7 +18321,7 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, 
unsigned long bandwidth,
 }
 
 /* qemuDomainBlockPullCommon consumes the reference on @vm */
-return qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags);
+return qemuDomainBlockPullCommon(vm, path, NULL, NULL, bandwidth, flags);
 }
 
 
-- 
2.24.1

[libvirt PATCH 5/6] qemu: block: 'top' support in construction of QMP block-stream

[Pavel Mores]

This commit only handles submission of block-stream command.  Since
block-stream is a job, we'll need to handle its completion as well
in an upcoming commit.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b3f97bf593..b1ffc68e6b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17410,7 +17410,15 @@ qemuDomainBlockPullCommon(virDomainObjPtr vm,
 !(backingPath = qemuBlockGetBackingStoreString(baseSource)))
 goto endjob;
 }
-device = disk->src->nodeformat;
+if (topSource) {
+device = topSource->nodeformat;
+if (qemuDomainStorageSourceAccessAllow(driver, vm, topSource, 
false, false) < 0) {
+virReportError(VIR_ERR_ACCESS_DENIED, "can't make 'top' 
writable");
+goto endjob;
+}
+} else {
+device = disk->src->nodeformat;
+}
 } else {
 device = job->name;
 }
@@ -18171,7 +18179,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char 
*path, const char *base,
 /* For normal rebase (enhanced blockpull), the common code handles
  * everything, including vm cleanup. */
 if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY))
-return qemuDomainBlockPullCommon(vm, path, base, NULL, bandwidth, 
flags);
+return qemuDomainBlockPullCommon(vm, path, base, top, bandwidth, 
flags);
 
 /* If we got here, we are doing a block copy rebase. */
 if (!(dest = virStorageSourceNew()))
@@ -18302,8 +18310,8 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, 
const char *destxml,
 
 
 static int
-qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long 
bandwidth,
-unsigned int flags)
+qemuDomainBlockPull(virDomainPtr dom, const char *path,
+unsigned long bandwidth, unsigned int flags)
 {
 virDomainObjPtr vm;
 virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1);
-- 
2.24.1

[libvirt PATCH 6/6] qemu: block: extend pull job completion with 'top' handling

[Pavel Mores]

'top' handling basically requires two changes: besides the obvious
adjustments to the code that fixes our backing chains (for both
active and inactive configuration) after a pull, we need to make
the top image read-only again if it's not identical with the
active layer (it had to be made writable before submitting QMP
block-stream to allow QEMU to fix the backing chain in top's
qcow2).

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_blockjob.c | 33 +
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index e19a2ad76b..2e53821d43 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -962,6 +962,7 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr 
driver,
 virDomainDiskDefPtr cfgdisk = NULL;
 virStorageSourcePtr cfgbase = NULL;
 virStorageSourcePtr cfgbaseparent = NULL;
+virStorageSourcePtr cfgtop = NULL;
 virStorageSourcePtr n;
 virStorageSourcePtr tmp;
 
@@ -994,16 +995,40 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr 
driver,
 }
 }
 
-tmp = job->disk->src->backingStore;
-job->disk->src->backingStore = job->data.pull.base;
+if (job->data.pull.top) {
+cfgtop = cfgdisk->src->backingStore;
+for (n = job->disk->src->backingStore; n && n != job->data.pull.top; n 
= n->backingStore) {
+if (cfgtop)
+cfgtop = cfgtop->backingStore;
+}
+}
+
+if (job->data.pull.top && job->data.pull.top != job->disk->src) {
+if (qemuDomainStorageSourceAccessAllow(driver, vm, job->data.pull.top, 
true, false) < 0) {
+virReportError(VIR_ERR_ACCESS_DENIED, "can't reset 'top' to 
read-only");
+}
+}
+
+if (job->data.pull.top) {
+tmp = job->data.pull.top->backingStore;
+job->data.pull.top->backingStore = job->data.pull.base;
+} else {
+tmp = job->disk->src->backingStore;
+job->disk->src->backingStore = job->data.pull.base;
+}
 if (baseparent)
 baseparent->backingStore = NULL;
 qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, tmp);
 virObjectUnref(tmp);
 
 if (cfgdisk) {
-tmp = cfgdisk->src->backingStore;
-cfgdisk->src->backingStore = cfgbase;
+if (job->data.pull.top) {
+tmp = cfgtop->backingStore;
+cfgtop->backingStore = cfgbase;
+} else {
+tmp = cfgdisk->src->backingStore;
+cfgdisk->src->backingStore = cfgbase;
+}
 if (cfgbaseparent)
 cfgbaseparent->backingStore = NULL;
 virObjectUnref(tmp);
-- 
2.24.1

[libvirt PATCH 3/6] qemu: block: blockpull param 'top' support in virsh proper

[Pavel Mores]

A new command-line option --top was added to virsh's blockpull command.
Similar to how --base is handled, in presence of --top the operation is
implemented internally as a rebase.  To that end, a corresponding new 'top'
parameter was added to virDomainBlockRebase().

Signed-off-by: Pavel Mores 
---
 include/libvirt/libvirt-domain.h |  4 ++--
 src/libvirt-domain.c |  5 +++--
 tools/virsh-domain.c | 14 +++---
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index b440818ec2..069d7f98ec 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2560,8 +2560,8 @@ typedef enum {
 } virDomainBlockRebaseFlags;
 
 int virDomainBlockRebase(virDomainPtr dom, const char *disk,
- const char *base, unsigned long bandwidth,
- unsigned int flags);
+ const char *base, const char *top,
+ unsigned long bandwidth, unsigned int flags);
 
 /**
  * virDomainBlockCopyFlags:
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 65813b68cc..1f9d1b5b84 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10150,6 +10150,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk,
  * @disk: path to the block device, or device shorthand
  * @base: path to backing file to keep, or device shorthand,
  *or NULL for no backing file
+ * @top: path to top file, or device shorthand, or NULL for no top
  * @bandwidth: (optional) specify bandwidth limit; flags determine the unit
  * @flags: bitwise-OR of virDomainBlockRebaseFlags
  *
@@ -10257,8 +10258,8 @@ virDomainBlockPull(virDomainPtr dom, const char *disk,
  */
 int
 virDomainBlockRebase(virDomainPtr dom, const char *disk,
- const char *base, unsigned long bandwidth,
- unsigned int flags)
+ const char *base, const char *top,
+ unsigned long bandwidth, unsigned int flags)
 {
 virConnectPtr conn;
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 9d0f7d68d2..47ccdd1b94 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2436,7 +2436,7 @@ cmdBlockcopy(vshControl *ctl, const vshCmd *cmd)
 if (bytes)
 flags |= VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES;
 
-if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0)
+if (virDomainBlockRebase(dom, path, dest, NULL, bandwidth, flags) < 0)
 goto cleanup;
 }
 
@@ -2765,6 +2765,10 @@ static const vshCmdOptDef opts_blockpull[] = {
  .type = VSH_OT_STRING,
  .help = N_("path of backing file in chain for a partial pull")
 },
+{.name = "top",
+ .type = VSH_OT_STRING,
+ .help = N_("path of top backing file in chain for a partial pull")
+},
 {.name = "wait",
  .type = VSH_OT_BOOL,
  .help = N_("wait for job to finish")
@@ -2804,6 +2808,7 @@ cmdBlockpull(vshControl *ctl, const vshCmd *cmd)
 int timeout = 0;
 const char *path = NULL;
 const char *base = NULL;
+const char *top = NULL;
 unsigned long bandwidth = 0;
 unsigned int flags = 0;
 virshBlockJobWaitDataPtr bjWait = NULL;
@@ -2817,6 +2822,9 @@ cmdBlockpull(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptStringReq(ctl, cmd, "base", ) < 0)
 return false;
 
+if (vshCommandOptStringReq(ctl, cmd, "top", ) < 0)
+return false;
+
 if (vshBlockJobOptionBandwidth(ctl, cmd, bytes, ) < 0)
 return false;
 
@@ -2834,11 +2842,11 @@ cmdBlockpull(vshControl *ctl, const vshCmd *cmd)
  verbose, timeout, async)))
 goto cleanup;
 
-if (base || flags) {
+if (base || top || flags) {
 if (bytes)
 flags |= VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES;
 
-if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0)
+if (virDomainBlockRebase(dom, path, base, top, bandwidth, flags) < 0)
 goto cleanup;
 } else {
 if (bytes)
-- 
2.24.1

[libvirt PATCH 0/6] [RFC] qemu: implement support for pulling into non-active layers

[Pavel Mores]

QEMU's block-stream command supports pulling ("streaming" in QMP lingo) into
arbitrary layer, however libvirt can only pull into an active layer.  This
series is a proof-of-concept work to extend libvirt's pull to support pulling
into non-active layers as well.

While this doesn't seem to be a much demanded feature by itself, it can be seen
as a prerequisite to implementing full support for external snapshots in
snapshot-delete(*) as that appears to be implementable in terms of a
full-featured pull.

As stated above, this is a proof-of-concept work so admittedly not much effort
went into fully polishing the series.  The expectation is that the series will
have to change heavily anyway before it's good enough to be pushed.

(*) https://bugzilla.redhat.com/show_bug.cgi?id=1519001

Pavel Mores (6):
  qemu: block: add 'top' parameter to qemuDomainBlockPullCommon()
  qemu: block: pull job extended with 'top' parameter
  qemu: block: blockpull param 'top' support in virsh proper
  qemu: block: blockpull param 'top' support in driver and RPC
  qemu: block: 'top' support in construction of QMP block-stream
  qemu: block: extend pull job completion with 'top' handling

 include/libvirt/libvirt-domain.h |  4 ++--
 src/driver-hypervisor.h  |  1 +
 src/libvirt-domain.c |  7 ---
 src/qemu/qemu_blockjob.c | 35 
 src/qemu/qemu_blockjob.h |  2 ++
 src/qemu/qemu_driver.c   | 33 +++---
 src/remote/remote_protocol.x |  1 +
 src/remote_protocol-structs  |  1 +
 tools/virsh-domain.c | 14 ++---
 9 files changed, 79 insertions(+), 19 deletions(-)

-- 
2.24.1

Re: [libvirt PATCH 4/8] conf: rename virNetDevSupportBandwidth to virNetDevSupportsBandwidth

[Pavel Mores]

lType);
>  }
>  
>  if (qosSupported && persistentNet) {
>  actualType = virDomainNetGetActualType(persistentNet);
> -qosSupported = virNetDevSupportBandwidth(actualType);
> +qosSupported = virNetDevSupportsBandwidth(actualType);
>  }
>  
>  if (!qosSupported) {
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 9800491755..ca18bb9e5f 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1342,7 +1342,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>  /* Set bandwidth or warn if requested and not supported. */
>  actualBandwidth = virDomainNetGetActualBandwidth(net);
>  if (actualBandwidth) {
> -if (virNetDevSupportBandwidth(actualType)) {
> +if (virNetDevSupportsBandwidth(actualType)) {
>  if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
>!virDomainNetTypeSharesHostView(net)) 
> < 0)
>  goto cleanup;
> @@ -4582,7 +4582,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>  return -1;
>  
>  if (virDomainNetGetActualBandwidth(net) &&
> -virNetDevSupportBandwidth(virDomainNetGetActualType(net)) &&
> +virNetDevSupportsBandwidth(virDomainNetGetActualType(net)) &&
>  virNetDevBandwidthClear(net->ifname) < 0)
>  VIR_WARN("cannot clear bandwidth setting for device : %s",
>   net->ifname);
> -- 
> 2.24.1
> 

Good change, I for one found the original, seemingly imperative name rather
confusing.

Somewhat related, there's a function called virNetDevBandwidthSupportsFloor()
in the same header file whose name seems a bit dubious as well.  Now admittedly
I was the one to add it :-) but on second thought, it doesn't really check a
bandwidth property, actually it checks a network property (it takes
virNetworkForwardType as its argument).  Would it make sense to rename it to
something like virNetworkSupportsFloor() (and then also probably move it a
better header file)?

Reviewed-by: Pavel Mores 

[libvirt PATCH v2 1/6] qemu: test if bandwidth has 'floor' factored out to separate function

[Pavel Mores]

This compound condition will be useful in several places so it makes sense
to give it a name for better readability.

Signed-off-by: Pavel Mores 
---
 src/conf/netdev_bandwidth_conf.h | 7 +++
 src/network/bridge_driver.c  | 8 
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h
index 0004e48a4a..5b5ed52566 100644
--- a/src/conf/netdev_bandwidth_conf.h
+++ b/src/conf/netdev_bandwidth_conf.h
@@ -23,6 +23,7 @@
 #include "virbuffer.h"
 #include "virxml.h"
 #include "domain_conf.h"
+#include "network_conf.h"
 
 int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
 unsigned int *class_id,
@@ -57,3 +58,9 @@ static inline bool virNetDevSupportBandwidth(virDomainNetType 
type)
 }
 return false;
 }
+
+
+static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b)
+{
+return b && b->in && b->in->floor != 0;
+}
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 94212eec77..14976c9821 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5065,8 +5065,7 @@ networkCheckBandwidth(virNetworkObjPtr obj,
 
 virMacAddrFormat(ifaceMac, ifmac);
 
-if (ifaceBand && ifaceBand->in && ifaceBand->in->floor &&
-!(netBand && netBand->in)) {
+if (virNetDevBandwidthHasFloor(ifaceBand) && !(netBand && netBand->in)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("Invalid use of 'floor' on interface with MAC "
  "address %s - network '%s' has no inbound QoS set"),
@@ -5079,8 +5078,9 @@ networkCheckBandwidth(virNetworkObjPtr obj,
 /* no QoS required, claim success */
 return 1;
 }
-if (((!ifaceBand || !ifaceBand->in || !ifaceBand->in->floor) &&
- (!oldBandwidth || !oldBandwidth->in || !oldBandwidth->in->floor))) {
+if (!virNetDevBandwidthHasFloor(ifaceBand) &&
+!virNetDevBandwidthHasFloor(oldBandwidth)) {
+
 VIR_DEBUG("No old/new interface bandwidth floor");
 /* no QoS required, claim success */
 return 1;
-- 
2.24.1

[libvirt PATCH v2 4/6] qemu: check if 'floor' is supported for given interface and network

[Pavel Mores]

Even if an interface of type 'network', setting 'floor' is only supported
if the network's forward type is nat, route, open or none.

Signed-off-by: Pavel Mores 
---
 src/network/bridge_driver.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 14976c9821..72220e1c64 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5065,6 +5065,16 @@ networkCheckBandwidth(virNetworkObjPtr obj,
 
 virMacAddrFormat(ifaceMac, ifmac);
 
+if (virNetDevBandwidthHasFloor(ifaceBand) &&
+!virNetDevSupportBandwidthFloor(def->forward.type)) {
+
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("Invalid use of 'floor' on interface with MAC address 
%s "
+ "- 'floor' is only supported for interface type 
'network' with forward type 'nat', 'route', 'open' or none"),
+   ifmac);
+return -1;
+}
+
 if (virNetDevBandwidthHasFloor(ifaceBand) && !(netBand && netBand->in)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("Invalid use of 'floor' on interface with MAC "
-- 
2.24.1

[libvirt PATCH v2 6/6] docs: QoS parameter 'floor' is supported for 'open' networks too

[Pavel Mores]

Relevant code seems to treat forward modes 'route', 'nat', 'open' and 'none'
the same but documentation hasn't reflected that so far.

Signed-off-by: Pavel Mores 
---
 docs/formatnetwork.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 2448fb09e7..3d807ecab6 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -631,7 +631,7 @@
   goes through one point where QoS decisions can take place, hence
   why this attribute works only for virtual networks for now
   (that is interface type='network'/ with a
-  forward type of route, nat, or no forward at all). Moreover, the
+  forward type of route, nat, open or no forward at all). Moreover, the
   virtual network the interface is connected to is required to have
   at least inbound QoS set (average at least). If
   using the floor attribute users don't need to specify
-- 
2.24.1

[libvirt PATCH v2 5/6] qemu: call networkPlugBandwidth() for all types of network

[Pavel Mores]

To fix the actual bug, it was necessary to make networkPlugBandwidth() be
called also for 'bridge'-type networks implemented using macvtap's 'bridge'
mode (previously it was only called for those implemented on top of an
existing bridge).

However, it seems beneficial to call it for other network types as well, at
least because it removes an inconsistency in types of bandwidth configuration
changes permissible in inactive and active domain configs.  It should also be
safe as the function pretty much amounts to NOP if no QoS is requested and the
new behaviour should not be any worse than before if it is.

Signed-off-by: Pavel Mores 
---
 src/network/bridge_driver.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 72220e1c64..c8f7f07acb 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4571,8 +4571,6 @@ networkAllocatePort(virNetworkObjPtr obj,
 return -1;
 }
 
-if (networkPlugBandwidth(obj, >mac, port->bandwidth, 
>class_id) < 0)
-return -1;
 break;
 
 case VIR_NETWORK_FORWARD_HOSTDEV: {
@@ -4637,8 +4635,6 @@ networkAllocatePort(virNetworkObjPtr obj,
 }
 }
 
-if (networkPlugBandwidth(obj, >mac, port->bandwidth, 
>class_id) < 0)
-return -1;
 break;
 }
 
@@ -4736,6 +4732,11 @@ networkAllocatePort(virNetworkObjPtr obj,
 return -1;
 }
 
+
+if (networkPlugBandwidth(obj, >mac, port->bandwidth,
+ >class_id) < 0)
+return -1;
+
 if (virNetworkObjMacMgrAdd(obj, driver->dnsmasqStateDir,
port->ownername, >mac) < 0)
 return -1;
-- 
2.24.1

[libvirt PATCH v2 2/6] qemu: add function to test if network supports setting 'floor'

[Pavel Mores]

This function will be useful in upcoming commits when code to check whether
a network can support 'floor' in the first place is introduced.

Signed-off-by: Pavel Mores 
---
 src/conf/netdev_bandwidth_conf.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h
index 5b5ed52566..0596d555e5 100644
--- a/src/conf/netdev_bandwidth_conf.h
+++ b/src/conf/netdev_bandwidth_conf.h
@@ -60,6 +60,26 @@ static inline bool 
virNetDevSupportBandwidth(virDomainNetType type)
 }
 
 
+static inline bool virNetDevSupportBandwidthFloor(virNetworkForwardType type)
+{
+switch (type) {
+case VIR_NETWORK_FORWARD_NONE:
+case VIR_NETWORK_FORWARD_NAT:
+case VIR_NETWORK_FORWARD_ROUTE:
+case VIR_NETWORK_FORWARD_OPEN:
+return true;
+case VIR_NETWORK_FORWARD_BRIDGE:
+case VIR_NETWORK_FORWARD_PRIVATE:
+case VIR_NETWORK_FORWARD_VEPA:
+case VIR_NETWORK_FORWARD_PASSTHROUGH:
+case VIR_NETWORK_FORWARD_HOSTDEV:
+case VIR_NETWORK_FORWARD_LAST:
+break;
+}
+return false;
+}
+
+
 static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b)
 {
 return b && b->in && b->in->floor != 0;
-- 
2.24.1

[libvirt PATCH v2 3/6] qemu: fail on attempt to set 'floor' if interface type is not 'network'

[Pavel Mores]

QoS 'floor' setting is documented to be only supported for interfaces of
type 'network'.  Fail with an error message on attempt to set 'floor' on
an interface of any other type.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2813f084cd..f686b858cf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11672,9 +11672,21 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
sizeof(*newBandwidth->out));
 }
 
-if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
-virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
-goto endjob;
+if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
+if (virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
+goto endjob;
+} else {
+if (virNetDevBandwidthHasFloor(bandwidth)) {
+char ifmac[VIR_MAC_STRING_BUFLEN];
+
+virMacAddrFormat(>mac, ifmac);
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("Invalid use of 'floor' on interface with MAC 
address %s "
+   "- 'floor' is only supported for interface type 
'network' with forward type 'nat', 'route', 'open' or none"),
+   ifmac);
+goto endjob;
+}
+}
 
 if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
   !virDomainNetTypeSharesHostView(net)) < 0) {
-- 
2.24.1

[libvirt PATCH v2 0/6] qemu: add stricter checks of permissibility of the QoS parameter 'floor'

[Pavel Mores]

v2 is mostly just integrating requests from Michal's review.  The initial two
commits introduce new utility functions to be used in the following two
commits.  The final two commits have no substantial changes since v1.

The only exception are long lines caused by error messages which stay unbroken
in v2 as per libvirt's contributor's guidelines (as was also pointed out during
review).

Pavel Mores (6):
  qemu: test if bandwidth has 'floor' factored out to separate function
  qemu: add function to test if network supports setting 'floor'
  qemu: fail on attempt to set 'floor' if interface type is not
'network'
  qemu: check if 'floor' is supported for given interface and network
  qemu: call networkPlugBandwidth() for all types of network
  docs: QoS parameter 'floor' is supported for 'open' networks too

 docs/formatnetwork.html.in   |  2 +-
 src/conf/netdev_bandwidth_conf.h | 27 +++
 src/network/bridge_driver.c  | 27 +++
 src/qemu/qemu_driver.c   | 18 +++---
 4 files changed, 62 insertions(+), 12 deletions(-)

-- 
2.24.1

[libvirt PATCH 4/5] docs: QoS parameter 'floor' is supported for 'open' networks too

[Pavel Mores]

Relevant code seems to treat forward modes 'route', 'nat', 'open' and none
the same but documentation hasn't reflected that so far.

Signed-off-by: Pavel Mores 
---
 docs/formatnetwork.html.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 2448fb09e7..3d807ecab6 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -631,7 +631,7 @@
   goes through one point where QoS decisions can take place, hence
   why this attribute works only for virtual networks for now
   (that is interface type='network'/ with a
-  forward type of route, nat, or no forward at all). Moreover, the
+  forward type of route, nat, open or no forward at all). Moreover, the
   virtual network the interface is connected to is required to have
   at least inbound QoS set (average at least). If
   using the floor attribute users don't need to specify
-- 
2.24.1

[libvirt PATCH 5/5] qemu: reuse convenience variable introduced in a00b97f27672b3

[Pavel Mores]

Since we have the result of a compound condition named now we can use it
to simplify another complex condition to make it more readable.

Signed-off-by: Pavel Mores 
---
 src/network/bridge_driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 513ae59e68..584c78cadb 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5081,8 +5081,7 @@ networkCheckBandwidth(virNetworkObjPtr obj,
 return -1;
 }
 
-if (ifaceBand && ifaceBand->in && ifaceBand->in->floor &&
-!(netBand && netBand->in)) {
+if (floorRequested && !(netBand && netBand->in)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("Invalid use of 'floor' on interface with MAC "
  "address %s - network '%s' has no inbound QoS set"),
-- 
2.24.1

[libvirt PATCH 3/5] qemu: call networkPlugBandwidth() for all types of network

[Pavel Mores]

To fix the actual bug, it was necessary to make networkPlugBandwidth() be
called also for 'bridge'-type networks implemented using macvtap's 'bridge'
mode (previously it was only called for those implemented on top of an
existing bridge).

However, it seems beneficial to call it for other network types as well,
at least because it removes an inconsistency in types of bandwidth
configuration changes permissible in inactive and active domain configs.

Signed-off-by: Pavel Mores 
---
 src/network/bridge_driver.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3b70e52afd..513ae59e68 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4571,8 +4571,6 @@ networkAllocatePort(virNetworkObjPtr obj,
 return -1;
 }
 
-if (networkPlugBandwidth(obj, >mac, port->bandwidth, 
>class_id) < 0)
-return -1;
 break;
 
 case VIR_NETWORK_FORWARD_HOSTDEV: {
@@ -4637,8 +4635,6 @@ networkAllocatePort(virNetworkObjPtr obj,
 }
 }
 
-if (networkPlugBandwidth(obj, >mac, port->bandwidth, 
>class_id) < 0)
-return -1;
 break;
 }
 
@@ -4736,6 +4732,9 @@ networkAllocatePort(virNetworkObjPtr obj,
 return -1;
 }
 
+if (networkPlugBandwidth(obj, >mac, port->bandwidth, 
>class_id) < 0)
+return -1;
+
 if (virNetworkObjMacMgrAdd(obj, driver->dnsmasqStateDir,
port->ownername, >mac) < 0)
 return -1;
-- 
2.24.1

[libvirt PATCH 1/5] qemu: fail on attempt to set 'floor' if interface type is not 'network'

[Pavel Mores]

QoS 'floor' setting is documented to be only supported for interfaces of
type 'network'.  Fail with an error message on attempt to set 'floor' on
an interface of any other type.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e69d083836..88fa56da42 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11672,9 +11672,16 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
sizeof(*newBandwidth->out));
 }
 
-if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
-virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
-goto endjob;
+if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
+if (virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
+goto endjob;
+} else {
+if (bandwidth->in && bandwidth->in->floor != 0) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("'floor' is only supported for interface type 
'network' with forward type 'nat', 'route', 'open' or none"));
+goto endjob;
+}
+}
 
 if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
   !virDomainNetTypeSharesHostView(net)) < 0) {
-- 
2.24.1

[libvirt PATCH 2/5] qemu: check if 'floor' is supported for given interface and network

[Pavel Mores]

Even if an interface of type 'network', setting 'floor' is only supported
if the network's forward type is nat, route, open or none.

Signed-off-by: Pavel Mores 
---
 src/network/bridge_driver.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 94212eec77..3b70e52afd 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5062,9 +5062,26 @@ networkCheckBandwidth(virNetworkObjPtr obj,
 unsigned long long tmp_floor_sum = virNetworkObjGetFloorSum(obj);
 unsigned long long tmp_new_rate = 0;
 char ifmac[VIR_MAC_STRING_BUFLEN];
+virNetworkForwardType fwdType;
+bool floorSupported;
+bool floorRequested;
 
 virMacAddrFormat(ifaceMac, ifmac);
 
+fwdType = def->forward.type;
+floorSupported = fwdType == VIR_NETWORK_FORWARD_NONE ||
+fwdType == VIR_NETWORK_FORWARD_NAT ||
+fwdType == VIR_NETWORK_FORWARD_ROUTE ||
+fwdType == VIR_NETWORK_FORWARD_OPEN;
+
+floorRequested = ifaceBand && ifaceBand->in && ifaceBand->in->floor != 0;
+
+if (floorRequested && !floorSupported) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("'floor' is only supported for interface type 
'network' with forward type 'nat', 'route', 'open' or none"));
+return -1;
+}
+
 if (ifaceBand && ifaceBand->in && ifaceBand->in->floor &&
 !(netBand && netBand->in)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
-- 
2.24.1

[libvirt PATCH 0/5] qemu: add stricter checks of permissibility of the QoS parameter 'floor'

[Pavel Mores]

Aims to fix

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

Libvirt previously silently accepted attempts to set 'floor' even for
direct bridge interface types where the parameter is not supported.  This
could happen when manipulating both inactive and active (e.g. via 'virsh
domiftune') domain configuration.

Pavel Mores (5):
  qemu: fail on attempt to set 'floor' if interface type is not
'network'
  qemu: check if 'floor' is supported for given interface and network
  qemu: call networkPlugBandwidth() for all types of network
  docs: QoS parameter 'floor' is supported for 'open' networks too
  qemu: reuse convenience variable introduced in a00b97f27672b3

 docs/formatnetwork.html.in  |  2 +-
 src/network/bridge_driver.c | 27 +--
 src/qemu/qemu_driver.c  | 13 ++---
 3 files changed, 32 insertions(+), 10 deletions(-)

-- 
2.24.1

Re: [libvirt] [PATCH 2/3] qemu: use g_autofree instead of VIR_FREE in qemuMonitorTextCreateSnapshot()

[Pavel Mores]

On Tue, Dec 17, 2019 at 10:55:24AM -0500, Cole Robinson wrote:
> On 12/6/19 4:11 AM, Pavel Mores wrote:
> > While at bugfixing, convert the whole function to the new-style memory
> > allocation handling.
> > 
> > Signed-off-by: Pavel Mores 
> > ---
> >  src/qemu/qemu_monitor_text.c | 18 ++
> >  1 file changed, 6 insertions(+), 12 deletions(-)
> > 
> 
> Reviewed-by: Cole Robinson 
> 
> Looks like this patch was missed. Pushed now

Thanks!  I dropped this patch from v2 intentionally because it was just a
clean-up of a previous patch which was dropped (for duplicating a recent
commit) and without which this one felt out of place and context in v2.

But on its own, this patch is still relevant so it's good you pushed it.

Thanks,

pvl

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 4/4] qemu: block: enable the snapshot image deletion feature

[Pavel Mores]

On Wed, Dec 11, 2019 at 08:34:38AM +0100, Peter Krempa wrote:
> On Tue, Dec 10, 2019 at 17:25:41 +0100, Pavel Mores wrote:
> > With all plumbing in place, we can now enable the new functionality.
> > 
> > Signed-off-by: Pavel Mores 
> > ---
> >  src/qemu/qemu_driver.c | 12 ++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 9c07b6b393..0cbc746c22 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -18544,10 +18544,10 @@ qemuDomainBlockCommit(virDomainPtr dom,
> >  bool persistjob = false;
> >  bool blockdev = false;
> >  
> > -/* XXX Add support for COMMIT_DELETE */
> >  virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
> >VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
> >VIR_DOMAIN_BLOCK_COMMIT_RELATIVE |
> > +  VIR_DOMAIN_BLOCK_COMMIT_DELETE |
> >VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1);
> >  
> >  if (!(vm = qemuDomainObjFromDomain(dom)))
> > @@ -18568,6 +18568,13 @@ qemuDomainBlockCommit(virDomainPtr dom,
> >  
> >  blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
> >  
> > +if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) {
> > +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > +   _("deleting committed images is only supported for 
> > VMs "
> > + "with blockdev enabled"));
> > +goto endjob;
> > +}
> 
> make[1]: Leaving directory '/home/pipo/build/libvirt/gcc/src'
> /home/pipo/libvirt/src/qemu/qemu_driver.c:18572:
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> /home/pipo/libvirt/src/qemu/qemu_driver.c-18573-   
> _("deleting committed images is only supported for VMs "
> /home/pipo/libvirt/src/qemu/qemu_driver.c-18574- 
> "with blockdev enabled"));
> build-aux/syntax-check.mk: found diagnostic without %
> make: *** [/home/pipo/libvirt/build-aux/syntax-check.mk:756: 
> sc_prohibit_diagnostic_without_format] Error 1
> 
> Please make sure you run 'make syntax-check' before submitting patches.
> 
> I'll fix this before pushing with with:
> 
> if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) {
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>_("deleting committed images is not supported by this 
> VM"));
> goto endjob;
> }
> 

Okay, my apologies, and thanks for the speedy review!

pvl

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v4 0/4] qemu: block: implement optional removal of committed snapshot images

[Pavel Mores]

This version incorporates Peter's feedback to v3.

Pavel Mores (4):
  qemu: block: propagate the delete flag to where it can actually be
used
  qemu: block: use the delete flag to delete snapshot images if
requested
  qemu: block: store the delete flag in libvirtd's status XML
  qemu: block: enable the snapshot image deletion feature

 src/qemu/qemu_blockjob.c  | 50 ++-
 src/qemu/qemu_blockjob.h  |  4 +-
 src/qemu/qemu_domain.c|  4 ++
 src/qemu/qemu_driver.c| 12 -
 .../blockjob-blockdev-in.xml  |  1 +
 5 files changed, 67 insertions(+), 4 deletions(-)

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v4 4/4] qemu: block: enable the snapshot image deletion feature

[Pavel Mores]

With all plumbing in place, we can now enable the new functionality.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9c07b6b393..0cbc746c22 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18544,10 +18544,10 @@ qemuDomainBlockCommit(virDomainPtr dom,
 bool persistjob = false;
 bool blockdev = false;
 
-/* XXX Add support for COMMIT_DELETE */
 virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
   VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
   VIR_DOMAIN_BLOCK_COMMIT_RELATIVE |
+  VIR_DOMAIN_BLOCK_COMMIT_DELETE |
   VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1);
 
 if (!(vm = qemuDomainObjFromDomain(dom)))
@@ -18568,6 +18568,13 @@ qemuDomainBlockCommit(virDomainPtr dom,
 
 blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
 
+if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+   _("deleting committed images is only supported for VMs "
+ "with blockdev enabled"));
+goto endjob;
+}
+
 /* Convert bandwidth MiB to bytes, if necessary */
 if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) {
 if (speed > LLONG_MAX >> 20) {
@@ -18686,7 +18693,8 @@ qemuDomainBlockCommit(virDomainPtr dom,
 goto endjob;
 
 if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource,
-  baseSource, false)))
+  baseSource,
+  flags & 
VIR_DOMAIN_BLOCK_COMMIT_DELETE)))
 goto endjob;
 
 disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v4 2/4] qemu: block: use the delete flag to delete snapshot images if requested

[Pavel Mores]

When blockcommit finishes successfully, one of the
qemuBlockJobProcessEventCompletedCommit() and
qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
This is where the delete flag (stored in qemuBlockJobCommitData since the
previous commit) can actually be used to delete the committed snapshot
images if requested.

We use virFileRemove() instead of a simple unlink() to cover the case where
the image to be removed is on an NFS volume.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_blockjob.c | 46 
 1 file changed, 46 insertions(+)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 718e311213..498e2a716f 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -1004,6 +1004,44 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr 
driver,
 }
 
 
+/**
+ * qemuBlockJobDeleteImages:
+ * @driver: qemu driver object
+ * @vm: domain object
+ * @disk: disk object that the chain to be deleted is associated with
+ * @top: top snapshot of the chain to be deleted
+ *
+ * Helper for removing snapshot images.  Intended for callers like
+ * qemuBlockJobProcessEventCompletedCommit() and
+ * qemuBlockJobProcessEventCompletedActiveCommit() as it relies on adjustments
+ * these functions perform on the 'backingStore' chain to function correctly.
+ *
+ * TODO look into removing backing store for non-local snapshots too
+ */
+static void
+qemuBlockJobDeleteImages(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr top)
+{
+virStorageSourcePtr p = top;
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+uid_t uid;
+gid_t gid;
+
+for (; p != NULL; p = p->backingStore) {
+if (virStorageSourceGetActualType(p) == VIR_STORAGE_TYPE_FILE) {
+
+qemuDomainGetImageIds(cfg, vm, p, disk->src, , );
+
+if (virFileRemove(p->path, uid, gid) < 0) {
+VIR_WARN("Unable to remove snapshot image file '%s' (%s)",
+ p->path, g_strerror(errno));
+}
+}
+}
+}
+
 /**
  * qemuBlockJobProcessEventCompletedCommit:
  * @driver: qemu driver object
@@ -1070,6 +1108,10 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr 
driver,
 job->data.commit.topparent->backingStore = job->data.commit.base;
 
 qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, 
job->data.commit.top);
+
+if (job->data.commit.deleteCommittedImages)
+qemuBlockJobDeleteImages(driver, vm, job->disk, job->data.commit.top);
+
 virObjectUnref(job->data.commit.top);
 job->data.commit.top = NULL;
 
@@ -1160,6 +1202,10 @@ 
qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr driver,
 job->disk->src->readonly = job->data.commit.top->readonly;
 
 qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, 
job->data.commit.top);
+
+if (job->data.commit.deleteCommittedImages)
+qemuBlockJobDeleteImages(driver, vm, job->disk, job->data.commit.top);
+
 virObjectUnref(job->data.commit.top);
 job->data.commit.top = NULL;
 /* the mirror element does not serve functional purpose for the commit job 
*/
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v4 3/4] qemu: block: store the delete flag in libvirtd's status XML

[Pavel Mores]

Since blockcommit is asynchronous, libvirtd can be restarted while the
operation runs.  To ensure the information necessary to finish up the job
is not lost, serialisation to and deserialisation from the status XML is
added.

To unittest this, the new element was only added to the active commit test,
the non-active commit test doesn't have the new element so as to test its
absence.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_domain.c   | 4 
 tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 1 +
 2 files changed, 5 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 767790bfc0..27926c7670 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2586,6 +2586,8 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void 
*payload,
 virBufferAsprintf(, "\n", 
job->data.commit.top->nodeformat);
 if (job->data.commit.topparent)
 virBufferAsprintf(, "\n", 
job->data.commit.topparent->nodeformat);
+if (job->data.commit.deleteCommittedImages)
+virBufferAddLit(, "\n");
 break;
 
 case QEMU_BLOCKJOB_TYPE_CREATE:
@@ -3185,6 +3187,8 @@ 
qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job,
  
"string(./base/@node)",
  
>data.commit.base,
  ctxt);
+if (virXPathNode("./deleteCommittedImages", ctxt))
+job->data.commit.deleteCommittedImages = true;
 if (!job->data.commit.top ||
 !job->data.commit.base)
 goto broken;
diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml 
b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
index 67ab099bd9..b5d62fd4ab 100644
--- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
+++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
@@ -242,6 +242,7 @@
   
   
   
+  
 
 
   
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v4 1/4] qemu: block: propagate the delete flag to where it can actually be used

[Pavel Mores]

Propagate the delete flag from qemuDomainBlockCommit() (which was just
ignoring it until now) to qemuBlockJobDiskNewCommit() where it can be
stored in the qemuBlockJobCommitData structure which holds information
necessary to finish the job asynchronously.

In the actual qemuBlockJobDiskNewCommit() in this commit, we temporarily
pass a literal 'false' to preserve the current behaviour until the whole
implementation of the feature is in place.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_blockjob.c | 4 +++-
 src/qemu/qemu_blockjob.h | 4 +++-
 src/qemu/qemu_driver.c   | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 81aa46c2fb..718e311213 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -282,7 +282,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
   virDomainDiskDefPtr disk,
   virStorageSourcePtr topparent,
   virStorageSourcePtr top,
-  virStorageSourcePtr base)
+  virStorageSourcePtr base,
+  bool delete_imgs)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 g_autoptr(qemuBlockJobData) job = NULL;
@@ -305,6 +306,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
 job->data.commit.topparent = topparent;
 job->data.commit.top = top;
 job->data.commit.base = base;
+job->data.commit.deleteCommittedImages = delete_imgs;
 
 if (qemuBlockJobRegister(job, vm, disk, true) < 0)
 return NULL;
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index 52b03aaf9e..42b973fe96 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -87,6 +87,7 @@ struct _qemuBlockJobCommitData {
 virStorageSourcePtr topparent;
 virStorageSourcePtr top;
 virStorageSourcePtr base;
+bool deleteCommittedImages;
 };
 
 
@@ -180,7 +181,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
   virDomainDiskDefPtr disk,
   virStorageSourcePtr topparent,
   virStorageSourcePtr top,
-  virStorageSourcePtr base);
+  virStorageSourcePtr base,
+  bool delete_imgs);
 
 qemuBlockJobDataPtr
 qemuBlockJobNewCreate(virDomainObjPtr vm,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9197dffadd..9c07b6b393 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18686,7 +18686,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
 goto endjob;
 
 if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource,
-  baseSource)))
+  baseSource, false)))
 goto endjob;
 
 disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/1] qemu: fix crash bug in snapshot revert

[Pavel Mores]

This is a significant rework of v1 as

- patch 1 was dropped because an equivalent change was accidentally committed
  in the meantime in 4c53267b70fc5c548b6530113c3f96870d8d7fc1
- patch 2 was dropped as it was just a mostly formal clean-up of patch 1
- patch 3 was redone using a different approach.

Patch 3 now takes the approach that was hinted at in the last paragraph of
v1's cover letter, more specifically it now fixes qemuProcessStop() rather
than its caller.  Digging into git history I located the commit that
introduced qemuProcessStop()'s problematic behaviour and after consulting with
Michal, we agreed that fixing qemuProcessStop() seems safe enough, and
definitely a better solution conceptually.

Pavel Mores (1):
  qemu: fix concurrency crash bug in snapshot revert

 src/qemu/qemu_domain.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/1] qemu: fix concurrency crash bug in snapshot revert

[Pavel Mores]

This commit aims to fix

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

The cause was apparently incorrect handling of jobs in snapshot revert code
which allowed a thread executing snapshot delete to begin job while
snapshot revert was still running on another thread.  The snapshot delete
thread then waited on a condition variable in qemuMonitorSend() while the
revert thread finished, changing (and effectively corrupting) the
qemuMonitor structure under the delete thread which led to its crash.

The incorrect handling of jobs in revert code was due to the fact that
although qemuDomainRevertToSnapshot() correctly begins a job at the start,
the job was implicitly ended when qemuProcessStop() was called because the
job lives in the QEMU driver's private data (qemuDomainObjPrivate) that
was purged during qemuProcessStop().

This fix prevents qemuProcessStop() from clearing jobs as the idea of
qemuProcessStop() clearing jobs seems wrong in the first place.  It was
(inadvertently) introduced in commit 888aa4b6b9db65e3db273341e79846, which
is effectively reverted by the second hunk of this commit.  To preserve
the desired effects of the faulty commit, the first hunk is included as
suggested by Michal.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_domain.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6f53e17b6a..e4a1bccc18 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -403,6 +403,8 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj,
 static void
 qemuDomainObjFreeJob(qemuDomainObjPrivatePtr priv)
 {
+qemuDomainObjResetJob(priv);
+qemuDomainObjResetAsyncJob(priv);
 VIR_FREE(priv->job.current);
 VIR_FREE(priv->job.completed);
 virCondDestroy(>job.cond);
@@ -2161,9 +2163,6 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr 
priv)
 virBitmapFree(priv->migrationCaps);
 priv->migrationCaps = NULL;
 
-qemuDomainObjResetJob(priv);
-qemuDomainObjResetAsyncJob(priv);
-
 virHashRemoveAll(priv->blockjobs);
 virHashRemoveAll(priv->dbusVMStates);
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] qemu: fix crash due to freeing an uninitialised pointer

[Pavel Mores]

On Fri, Dec 06, 2019 at 02:49:26PM +0100, Michal Privoznik wrote:
> On 12/6/19 10:11 AM, Pavel Mores wrote:
> > The bug was fairly though not 100% reproducible, presumably due to the fact
> > that some of the time, 'safename' might turn out NULL by chance, in which 
> > case
> > freeing it is OK.
> > 
> > Signed-off-by: Pavel Mores 
> > ---
> >   src/qemu/qemu_monitor_text.c | 2 --
> >   1 file changed, 2 deletions(-)
> 
> D'oh! I've came around this bug in the morning when playing with libvirt-php
> too and merged patch around the same time as you posted this fix.
> 
> https://www.redhat.com/archives/libvir-list/2019-December/msg00341.html
> 
> Sorry,

No problem,  what's important is that it's fixed. :-)

pvl

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu: remove nested branching to enhance readability

[Pavel Mores]

This is a follow-up to patch series posted in

https://www.redhat.com/archives/libvir-list/2019-November/msg01180.html

It implements a suggestion made by Cole in

https://www.redhat.com/archives/libvir-list/2019-November/msg01207.html

and discussed in follow-up messages as there were no objections to the
change.

The aim is to make the code more readable by replacing nested branching
with a flat structure.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_domain.c | 19 ---
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d1596a28ca..5e4eede3c2 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7914,19 +7914,16 @@ static int
 qemuDomainDefaultVideoDevice(const virDomainDef *def,
   virQEMUCapsPtr qemuCaps)
 {
-if (ARCH_IS_PPC64(def->os.arch)) {
+if (ARCH_IS_PPC64(def->os.arch))
 return VIR_DOMAIN_VIDEO_TYPE_VGA;
-} else if (qemuDomainIsARMVirt(def) ||
-   qemuDomainIsRISCVVirt(def) ||
-   ARCH_IS_S390(def->os.arch)) {
+if (qemuDomainIsARMVirt(def) || qemuDomainIsRISCVVirt(def) ||
+ARCH_IS_S390(def->os.arch))
 return VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
-} else {
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA))
-return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA))
-return VIR_DOMAIN_VIDEO_TYPE_VGA;
-return VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
-}
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA))
+return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA))
+return VIR_DOMAIN_VIDEO_TYPE_VGA;
+return VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
 }
 
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/3] qemu: fix crash bugs in snapshot revert

[Pavel Mores]

The aim of this series is to fix

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

however before getting to that we first need to fix an unrelated (and much
more recent) bug in patch 1.  We clean up the fix in patch 2 by converting the
whole function to the new allocation idioms.

The actual reported bug is then fixed in patch 3.

Pavel Mores (3):
  qemu: fix crash due to freeing an uninitialised pointer
  qemu: use g_autofree instead of VIR_FREE in
qemuMonitorTextCreateSnapshot()
  qemu: fix concurrency crash bug in snapshot revert

 src/qemu/qemu_driver.c   | 17 ++---
 src/qemu/qemu_monitor_text.c | 20 ++--
 2 files changed, 20 insertions(+), 17 deletions(-)

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/3] qemu: use g_autofree instead of VIR_FREE in qemuMonitorTextCreateSnapshot()

[Pavel Mores]

While at bugfixing, convert the whole function to the new-style memory
allocation handling.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_monitor_text.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index b387235821..7586ba4c54 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -125,14 +125,13 @@ int
 qemuMonitorTextCreateSnapshot(qemuMonitorPtr mon,
   const char *name)
 {
-char *cmd = NULL;
-char *reply = NULL;
-int ret = -1;
+g_autofree char *cmd = NULL;
+g_autofree char *reply = NULL;
 
 cmd = g_strdup_printf("savevm \"%s\"", name);
 
 if (qemuMonitorJSONHumanCommand(mon, cmd, ))
-goto cleanup;
+return -1;
 
 if (strstr(reply, "Error while creating snapshot") ||
 strstr(reply, "Could not open VM state file") ||
@@ -141,19 +140,14 @@ qemuMonitorTextCreateSnapshot(qemuMonitorPtr mon,
 (strstr(reply, "Error") && strstr(reply, "while writing VM"))) {
 virReportError(VIR_ERR_OPERATION_FAILED,
_("Failed to take snapshot: %s"), reply);
-goto cleanup;
+return -1;
 } else if (strstr(reply, "No block device can accept snapshots")) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("this domain does not have a device to take 
snapshots"));
-goto cleanup;
+return -1;
 }
 
-ret = 0;
-
- cleanup:
-VIR_FREE(cmd);
-VIR_FREE(reply);
-return ret;
+return 0;
 }
 
 int qemuMonitorTextLoadSnapshot(qemuMonitorPtr mon, const char *name)
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/3] qemu: fix concurrency crash bug in snapshot revert

[Pavel Mores]

Although qemuDomainRevertToSnapshot() correctly begins a job at the start,
the job is implicitly ended if qemuProcessStop() is called because it lives
in the QEMU driver's private data that is purged during qemuProcessStop().
This commit restores the job by calling qemuProcessBeginJob() after
qemuProcessStop() invocations.

The first chunk is meant to fix a reported bug (see below).  The bug's
reproducibility on my machine was initially way lower than the reported
50% (more like 5%) but could be boosted to pretty much 100% by having
virt-manager connected, displaying the testing domain in viewer.  With the
fix, the bug cannot be reproduced on my machine under any scenario I could
think of.

The actual crash was due to the thread performing revert which, not
acquiring a job properly, was able to change the QEMU monitor structure
under the thread performing snapshot delete while it was waiting on a
condition variable.

An interesting question is whether this commit takes the right approach
to the fix.  In particular, qemuProcessStop() arguably should not clear
jobs, in which case the right thing to do would be to fix
qemuProcessStop().  However, qemuProcessStop() has about twenty callers,
and while none of them seems vulnerable to the kind of problems caused by
clearing jobs (unlike qemuDomainRevertToSnapshot() who starts QEMU again
right after stopping it), some of them might rely on jobs being cleared
(this is not always easy to check conclusively).  All in all, this commit's
solution seems to be the least bad fix which doesn't require a potentially
risky refactor.

https://bugzilla.redhat.com/show_bug.cgi?id=1610207
Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_driver.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 18bd0101e7..b3769cc479 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16637,9 +16637,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
  VIR_DOMAIN_EVENT_STOPPED,
  detail);
 virObjectEventStateQueue(driver->domainEventState, event);
-/* Start after stop won't be an async start job, so
- * reset to none */
-jobType = QEMU_ASYNC_JOB_NONE;
+/* We have to begin a new job as the original one (begun
+ * near the top of this function) was lost during the purge
+ * of private data in qemuProcessStop() above.
+ */
+if (qemuProcessBeginJob(driver, vm,
+
VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT,
+flags) < 0) {
+goto cleanup;
+}
 goto load;
 }
 }
@@ -16774,6 +16780,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 event = virDomainEventLifecycleNewFromObj(vm,
  VIR_DOMAIN_EVENT_STOPPED,
  detail);
+if (qemuProcessBeginJob(driver, vm,
+VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT,
+flags) < 0) {
+goto cleanup;
+}
 }
 
 if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) {
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/3] qemu: fix crash due to freeing an uninitialised pointer

[Pavel Mores]

The bug was fairly though not 100% reproducible, presumably due to the fact
that some of the time, 'safename' might turn out NULL by chance, in which case
freeing it is OK.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_monitor_text.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 9054682d60..b387235821 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -161,7 +161,6 @@ int qemuMonitorTextLoadSnapshot(qemuMonitorPtr mon, const 
char *name)
 char *cmd = NULL;
 char *reply = NULL;
 int ret = -1;
-char *safename;
 
 cmd = g_strdup_printf("loadvm \"%s\"", name);
 
@@ -194,7 +193,6 @@ int qemuMonitorTextLoadSnapshot(qemuMonitorPtr mon, const 
char *name)
 ret = 0;
 
  cleanup:
-VIR_FREE(safename);
 VIR_FREE(cmd);
 VIR_FREE(reply);
 return ret;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/4] qemu: fix type of default video device

[Pavel Mores]

On Mon, Nov 25, 2019 at 10:04:58AM -0500, Cole Robinson wrote:
> On 11/25/19 9:40 AM, Pavel Mores wrote:
> > On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
> >> On 11/25/19 5:54 AM, Pavel Mores wrote:
> >>> This new version mostly integrates Cole's comments about the second 
> >>> version.
> >>> Refactoring and behaviour change are now separate commits.  Tests succeed 
> >>> for
> >>> every individual patch in the series.
> >>>
> >>> Pavel Mores (4):
> >>>   qemu: default video device type selection algoritm moved into its own
> >>> function
> >>>   qemu: prepare existing test for change of the default video device
> >>> type
> >>>   qemu: the actual change of default video devide type selection
> >>> algorithm
> >>>   qemu: added tests of the new default video type selection algorithm
> >>
> >> Reviewed-by: Cole Robinson 
> >>
> >> and pushed now.
> >>
> >> patch #3 still had some style issues I pointed out in the previous
> >> review: using 'else' when it isn't necessary, because every block
> >> returns. Better to use the style of qemuDomainDefaultNetModel which
> >> saves having to do any nested logic. I'll be happy to review a follow up
> >> cleanup patch for that
> > 
> > Oh, now I see I changed my own additions but overlooked the style of
> > pre-existing code.  Sorry about that - I guess I don't have an eye for this 
> > as
> > personally I'd probably slightly prefer the else-based style.
> > 
> 
> The main issue with the code as implemented is that the third block is
> indented under an 'else' when it doesn't need to be. If we followed that
> pattern to the extreme the code would look like
> 
> if (condition1) {
> return foo;
> } else {
> if (condition2) {
> return bar;
> } else {
> if (condition3)
> return baz;
> }
> }
> 
> instead of
> 
> if (condition1)
> return foo;
> if (condition2)
> return bar;
> if (condition3)
> return baz;

I agree that the way the code is now, the flat style is more readable.  I just
feel that the additional structure provided by nesting could come handy if the
algorithm has be changed again in a way that made it impossible to have just a
return in every branch.

If we can reasonably assume that that's not going to happen, and if the
original author doesn't object (cc'ing him), I'm happy to make the change.

Thanks,

pvl

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3 0/4] qemu: fix type of default video device

[Pavel Mores]

On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
> On 11/25/19 5:54 AM, Pavel Mores wrote:
> > This new version mostly integrates Cole's comments about the second version.
> > Refactoring and behaviour change are now separate commits.  Tests succeed 
> > for
> > every individual patch in the series.
> > 
> > Pavel Mores (4):
> >   qemu: default video device type selection algoritm moved into its own
> > function
> >   qemu: prepare existing test for change of the default video device
> > type
> >   qemu: the actual change of default video devide type selection
> > algorithm
> >   qemu: added tests of the new default video type selection algorithm
> 
> Reviewed-by: Cole Robinson 
> 
> and pushed now.
> 
> patch #3 still had some style issues I pointed out in the previous
> review: using 'else' when it isn't necessary, because every block
> returns. Better to use the style of qemuDomainDefaultNetModel which
> saves having to do any nested logic. I'll be happy to review a follow up
> cleanup patch for that

Oh, now I see I changed my own additions but overlooked the style of
pre-existing code.  Sorry about that - I guess I don't have an eye for this as
personally I'd probably slightly prefer the else-based style.

> Minor bit: I would have squashed patch #2 + #3 together, maybe only
> separating them if there was lots of test suite churn needed, but I'm
> not sure if others agree.
> 
> Thanks,
> Cole
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 1/4] qemu: default video device type selection algoritm moved into its own function

[Pavel Mores]

The default video device type selection algorithm we're about to deploy will
increase the amount of code dedicated to the task by amount enough to warrant
factoring the whole thing into its own function so as not to pollute the
caller qemuDomainDeviceVideoDefPostParse().  Do it now so that the actual
algorithm change later on is in a clean commit by itself and easy to review.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_domain.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d56dfa9f00..26da41f9ee 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7876,20 +7876,26 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
 }
 
 
+static int
+qemuDomainDefaultVideoDevice(const virDomainDef *def)
+{
+if (ARCH_IS_PPC64(def->os.arch))
+return VIR_DOMAIN_VIDEO_TYPE_VGA;
+else if (qemuDomainIsARMVirt(def) ||
+ qemuDomainIsRISCVVirt(def) ||
+ ARCH_IS_S390(def->os.arch))
+return VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
+else
+return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+}
+
+
 static int
 qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
   const virDomainDef *def)
 {
-if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
-if (ARCH_IS_PPC64(def->os.arch))
-video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
-else if (qemuDomainIsARMVirt(def) ||
- qemuDomainIsRISCVVirt(def) ||
- ARCH_IS_S390(def->os.arch))
-video->type = VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
-else
-video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
-}
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT)
+video->type = qemuDomainDefaultVideoDevice(def);
 
 if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
 !video->vgamem) {
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 0/4] qemu: fix type of default video device

[Pavel Mores]

This new version mostly integrates Cole's comments about the second version.
Refactoring and behaviour change are now separate commits.  Tests succeed for
every individual patch in the series.

Pavel Mores (4):
  qemu: default video device type selection algoritm moved into its own
function
  qemu: prepare existing test for change of the default video device
type
  qemu: the actual change of default video devide type selection
algorithm
  qemu: added tests of the new default video type selection algorithm

 src/qemu/qemu_domain.c| 37 ++--
 .../default-video-type-aarch64.xml| 16 +++
 .../default-video-type-ppc64.xml  | 16 +++
 .../default-video-type-riscv64.xml| 16 +++
 .../default-video-type-s390x.xml  | 16 +++
 .../default-video-type-x86_64-caps-test-0.xml | 17 
 .../default-video-type-x86_64-caps-test-1.xml | 17 
 tests/qemuxml2argvtest.c  |  1 +
 ...ault-video-type-aarch64.aarch64-latest.xml | 42 +++
 .../default-video-type-ppc64.ppc64-latest.xml | 31 ++
 ...ault-video-type-riscv64.riscv64-latest.xml | 39 +
 .../default-video-type-s390x.s390x-latest.xml | 32 ++
 .../default-video-type-x86_64-caps-test-0.xml | 31 ++
 .../default-video-type-x86_64-caps-test-1.xml | 31 ++
 tests/qemuxml2xmltest.c   | 10 -
 15 files changed, 339 insertions(+), 13 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/default-video-type-aarch64.xml
 create mode 100644 tests/qemuxml2argvdata/default-video-type-ppc64.xml
 create mode 100644 tests/qemuxml2argvdata/default-video-type-riscv64.xml
 create mode 100644 tests/qemuxml2argvdata/default-video-type-s390x.xml
 create mode 100644 
tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.xml
 create mode 100644 
tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-1.xml
 create mode 100644 
tests/qemuxml2xmloutdata/default-video-type-aarch64.aarch64-latest.xml
 create mode 100644 
tests/qemuxml2xmloutdata/default-video-type-ppc64.ppc64-latest.xml
 create mode 100644 
tests/qemuxml2xmloutdata/default-video-type-riscv64.riscv64-latest.xml
 create mode 100644 
tests/qemuxml2xmloutdata/default-video-type-s390x.s390x-latest.xml
 create mode 100644 
tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-0.xml
 create mode 100644 
tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-1.xml

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3 3/4] qemu: the actual change of default video devide type selection algorithm

[Pavel Mores]

If a graphics device was added to XML that had no video device, libvirt
automatically added a video device which was always of type 'cirrus' on
x86_64, even if the underlying qemu didn't support cirrus.

This patch refines a bit the decision about the type of the video device.
Based on QEMU capabilities, cirrus is still preferred but only added if
QEMU supports it, otherwise VGA is used if supported by QEMU.  There is now
no fallback as libvirt only aspires to generate a basic working config and
leaves anything more specific up to higher-level management tools.

Signed-off-by: Pavel Mores 
---
 src/qemu/qemu_domain.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 26da41f9ee..18b6f1ea1c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7877,25 +7877,32 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
 
 
 static int
-qemuDomainDefaultVideoDevice(const virDomainDef *def)
+qemuDomainDefaultVideoDevice(const virDomainDef *def,
+  virQEMUCapsPtr qemuCaps)
 {
-if (ARCH_IS_PPC64(def->os.arch))
+if (ARCH_IS_PPC64(def->os.arch)) {
 return VIR_DOMAIN_VIDEO_TYPE_VGA;
-else if (qemuDomainIsARMVirt(def) ||
- qemuDomainIsRISCVVirt(def) ||
- ARCH_IS_S390(def->os.arch))
+} else if (qemuDomainIsARMVirt(def) ||
+   qemuDomainIsRISCVVirt(def) ||
+   ARCH_IS_S390(def->os.arch)) {
 return VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
-else
-return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+} else {
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA))
+return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA))
+return VIR_DOMAIN_VIDEO_TYPE_VGA;
+return VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
+}
 }
 
 
 static int
 qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
-  const virDomainDef *def)
+  const virDomainDef *def,
+  virQEMUCapsPtr qemuCaps)
 {
 if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT)
-video->type = qemuDomainDefaultVideoDevice(def);
+video->type = qemuDomainDefaultVideoDevice(def, qemuCaps);
 
 if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
 !video->vgamem) {
@@ -7989,7 +7996,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 break;
 
 case VIR_DOMAIN_DEVICE_VIDEO:
-ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def);
+ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def, 
qemuCaps);
 break;
 
 case VIR_DOMAIN_DEVICE_PANIC:
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list