[libvirt] [PATCH 8/9] qemu: Add -blockdev support for block pull job

Ján Tomko jtomko at redhat.com
Fri Jul 26 12:12:57 UTC 2019


On Wed, Jul 24, 2019 at 11:07:35PM +0200, Peter Krempa wrote:
>Introduce the handler for finalizing a block pull job which will allow
>to use it with blockdev.
>
>This patch also contains some additional machinery which is required to
>store all the relevant job data in the status XML which will also be
>reused with other block job types.
>
>Signed-off-by: Peter Krempa <pkrempa at redhat.com>
>---
> src/qemu/qemu_blockjob.c                      | 191 +++++++++++++++++-
> src/qemu/qemu_blockjob.h                      |  18 ++
> src/qemu/qemu_domain.c                        |  77 +++++++
> src/qemu/qemu_driver.c                        |  33 ++-
> .../blockjob-blockdev-in.xml                  |   4 +
> 5 files changed, 313 insertions(+), 10 deletions(-)
>
>diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
>index 0c0ae89f10..a29af7ec48 100644
>--- a/src/qemu/qemu_blockjob.c
>+++ b/src/qemu/qemu_blockjob.c
>+/**
>+ * qemuBlockJobProcessEventCompletedPull:
>+ * @driver: qemu driver object
>+ * @vm: domain object
>+ * @job: job data
>+ * @asyncJob: qemu asynchronous job type (for monitor interaction)
>+ *
>+ * This function executes the finalizing steps after a successful block pull job
>+ * (block-stream in qemu terminology. The pull job copies all the data from the
>+ * images in the backing chain up to the 'base' image. The 'base' image becomes
>+ * the backing store of the active top level image. If 'base' was not used
>+ * everything is pulled into the top level image and the top level image will
>+ * cease to have backing store. All intermediate images between the active image
>+ * and base image are no longer required and can be unplugged.
>+ */
>+static void
>+qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
>+                                      virDomainObjPtr vm,
>+                                      qemuBlockJobDataPtr job,
>+                                      qemuDomainAsyncJob asyncJob)
>+{
>+    virStorageSourcePtr baseparent = NULL;
>+    virDomainDiskDefPtr cfgdisk = NULL;
>+    virStorageSourcePtr cfgbase = NULL;
>+    virStorageSourcePtr cfgbaseparent = NULL;
>+    virStorageSourcePtr n;
>+    virStorageSourcePtr tmp;
>+
>+    VIR_DEBUG("pull job '%s' on VM '%s' completed", job->name, vm->def->name);
>+
>+    /* if the job isn't associated with a disk there's nothing to do */
>+    if (!job->disk)
>+        return;
>+
>+    if ((cfgdisk = qemuBlockJobGetConfigDisk(vm, job->disk, job->data.pull.base)))
>+        cfgbase = cfgdisk->src->backingStore;
>+

Consider:
      cfgdisk = ...();
      if (cfgdisk)
          cfgbase = ...;
      else
          ...();

>+    if (!cfgdisk)
>+        qemuBlockJobClearConfigChain(vm, job->disk);
>+
>+    /* when pulling if 'base' is right below the top image we don't have to modify it */
>+    if (job->disk->src->backingStore == job->data.pull.base)
>+        return;
>+
>+    if (job->data.pull.base) {
>+        for (n = job->disk->src->backingStore; n && n != job->data.pull.base; n = n->backingStore) {
>+            /* find the image on top of 'base' */
>+
>+            if (cfgbase) {
>+                cfgbaseparent = cfgbase;
>+                cfgbase = cfgbase->backingStore;
>+            }
>+
>+            baseparent = n;
>+        }
>+    }
>+
>+    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;

You can unref tmp directly here

>+        cfgdisk->src->backingStore = cfgbase;
>+        if (cfgbaseparent)
>+            cfgbaseparent->backingStore = NULL;
>+        virObjectUnref(tmp);
>+    }
>+}
>+
>+
> static void
> qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job,
>                                             virQEMUDriverPtr driver,
>                                             virDomainObjPtr vm,
>-                                            qemuDomainAsyncJob asyncJob ATTRIBUTE_UNUSED)
>+                                            qemuDomainAsyncJob asyncJob)
> {
>     switch ((qemuBlockjobState) job->newstate) {
>     case QEMU_BLOCKJOB_STATE_COMPLETED:
>         switch ((qemuBlockJobType) job->type) {
>         case QEMU_BLOCKJOB_TYPE_PULL:
>+            qemuBlockJobProcessEventCompletedPull(driver, vm, job, asyncJob);
>+            break;
>+
>         case QEMU_BLOCKJOB_TYPE_COMMIT:
>         case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
>         case QEMU_BLOCKJOB_TYPE_COPY:
>diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
>index 3299207610..d5848fb72c 100644
>--- a/src/qemu/qemu_blockjob.h
>+++ b/src/qemu/qemu_blockjob.h
>@@ -68,6 +68,15 @@ verify((int)QEMU_BLOCKJOB_TYPE_INTERNAL == VIR_DOMAIN_BLOCK_JOB_TYPE_LAST);
>
> VIR_ENUM_DECL(qemuBlockjob);
>
>+
>+typedef struct _qemuBlockJobPullData qemuBlockJobPullData;
>+typedef qemuBlockJobPullData *qemuBlockJobDataPullPtr;
>+
>+struct _qemuBlockJobPullData {
>+    virStorageSourcePtr base;
>+};
>+
>+
> typedef struct _qemuBlockJobData qemuBlockJobData;
> typedef qemuBlockJobData *qemuBlockJobDataPtr;
>
>@@ -80,6 +89,10 @@ struct _qemuBlockJobData {
>     virStorageSourcePtr chain; /* Reference to the chain the job operates on. */
>     virStorageSourcePtr mirrorChain; /* reference to 'mirror' part of the job */
>
>+    union {
>+        qemuBlockJobPullData pull;
>+    } data;
>+
>     int type; /* qemuBlockJobType */
>     int state; /* qemuBlockjobState */
>     char *errmsg;
>@@ -114,6 +127,11 @@ void
> qemuBlockJobDiskRegisterMirror(qemuBlockJobDataPtr job)
>     ATTRIBUTE_NONNULL(1);
>
>+qemuBlockJobDataPtr
>+qemuBlockJobDiskNewPull(virDomainObjPtr vm,
>+                        virDomainDiskDefPtr disk,
>+                        virStorageSourcePtr base);
>+
> qemuBlockJobDataPtr
> qemuBlockJobDiskGetJob(virDomainDiskDefPtr disk)
>     ATTRIBUTE_NONNULL(1);
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index c508f55287..ec1dda4870 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -2390,6 +2390,21 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload,
>             return -1;
>     }
>
>+    switch ((qemuBlockJobType) job->type) {
>+        case QEMU_BLOCKJOB_TYPE_PULL:
>+            if (job->data.pull.base)
>+                virBufferAsprintf(&childBuf, "<base node='%s'/>\n", job->data.pull.base->nodeformat);
>+            break;
>+
>+        case QEMU_BLOCKJOB_TYPE_COMMIT:
>+        case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
>+        case QEMU_BLOCKJOB_TYPE_COPY:
>+        case QEMU_BLOCKJOB_TYPE_NONE:
>+        case QEMU_BLOCKJOB_TYPE_INTERNAL:
>+        case QEMU_BLOCKJOB_TYPE_LAST:
>+            break;
>+    }
>+
>     return virXMLFormatElement(data->buf, "blockjob", &attrBuf, &childBuf);
> }
>
>@@ -2793,6 +2808,64 @@ qemuDomainObjPrivateXMLParseBlockjobChain(xmlNodePtr node,
> }
>
>
>+static void
>+qemuDomainObjPrivateXMLParseBlockjobNodename(qemuBlockJobDataPtr job,
>+                                             const char *xpath,
>+                                             virStorageSourcePtr *src,
>+                                             xmlXPathContextPtr ctxt)
>+{
>+    VIR_AUTOFREE(char *) nodename = NULL;
>+
>+    *src = NULL;
>+
>+    if (!(nodename = virXPathString(xpath, ctxt)))
>+        return;
>+
>+    if (job->disk &&
>+        (*src = virStorageSourceFindByNodeName(job->disk->src, nodename, NULL)))
>+        return;
>+
>+    if (job->chain &&
>+        (*src = virStorageSourceFindByNodeName(job->chain, nodename, NULL)))
>+        return;
>+
>+    if (job->mirrorChain &&
>+        (*src = virStorageSourceFindByNodeName(job->mirrorChain, nodename, NULL)))
>+        return;
>+
>+    /* the node was in the XML but was not found in the job definitions */
>+    VIR_DEBUG("marking block job '%s' as invalid: node name '%s' missing",
>+              job->name, nodename);
>+    job->invalidData = true;
>+}
>+
>+
>+static void
>+qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job,
>+                                                 xmlXPathContextPtr ctxt)
>+{
>+    switch ((qemuBlockJobType) job->type) {
>+        case QEMU_BLOCKJOB_TYPE_PULL:
>+            qemuDomainObjPrivateXMLParseBlockjobNodename(job,
>+                                                         "string(./base/@node)",
>+                                                         &job->data.pull.base,
>+                                                         ctxt);
>+            /* base is not present if pulling everything */
>+            break;
>+
>+        case QEMU_BLOCKJOB_TYPE_COMMIT:
>+        case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT:
>+        case QEMU_BLOCKJOB_TYPE_COPY:
>+        case QEMU_BLOCKJOB_TYPE_NONE:
>+        case QEMU_BLOCKJOB_TYPE_INTERNAL:

>+        case QEMU_BLOCKJOB_TYPE_LAST:

virReportEnumRangeError

>+            break;
>+    }
>+
>+    return;
>+}
>+
>+
> static int
> qemuDomainObjPrivateXMLParseBlockjobData(virDomainObjPtr vm,
>                                          xmlNodePtr node,
>@@ -2863,10 +2936,14 @@ qemuDomainObjPrivateXMLParseBlockjobData(virDomainObjPtr vm,
>     job->errmsg = virXPathString("string(./errmsg)", ctxt);
>     job->invalidData = invalidData;
>     job->disk = disk;
>+    if (invalidData)
>+        VIR_DEBUG("marking block job '%s' as invalid: basic data broken", job->name);
>

This hunk belongs to a separate patch.

>     if (mirror)
>         qemuBlockJobDiskRegisterMirror(job);
>
>+    qemuDomainObjPrivateXMLParseBlockjobDataSpecific(job, ctxt);
>+

Possibly this one too.

>     if (qemuBlockJobRegister(job, vm, disk, false) < 0)
>         return -1;
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index b6d705b679..705c1a06c0 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -17040,7 +17040,8 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
>                           unsigned int flags)
> {
>     qemuDomainObjPrivatePtr priv = vm->privateData;
>-    VIR_AUTOFREE(char *) device = NULL;
>+    const char *device = NULL;
>+    const char *jobname = NULL;
>     virDomainDiskDefPtr disk;
>     virStorageSourcePtr baseSource = NULL;
>     unsigned int baseIndex = 0;
>@@ -17048,6 +17049,8 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
>     VIR_AUTOFREE(char *) backingPath = NULL;
>     unsigned long long speed = bandwidth;
>     qemuBlockJobDataPtr job = NULL;
>+    bool persistjob = false;
>+    const char *nodebase = NULL;
>     int ret = -1;
>
>     if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) {
>@@ -17066,9 +17069,6 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
>     if (!(disk = qemuDomainDiskByName(vm->def, path)))
>         goto endjob;
>
>-    if (!(device = qemuAliasDiskDriveFromDisk(disk)))
>-        goto endjob;
>-
>     if (qemuDomainDiskBlockJobIsActive(disk))
>         goto endjob;
>
>@@ -17111,16 +17111,31 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
>         speed <<= 20;
>     }
>
>-    if (!(job = qemuBlockJobDiskNew(vm, disk, QEMU_BLOCKJOB_TYPE_PULL, device)))
>+    if (!(job = qemuBlockJobDiskNewPull(vm, disk, baseSource)))
>         goto endjob;
>
>+    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) {

Please put this capability in a 'blockdev' bool and use it instead of
jobname below:

>+        jobname = job->name;
>+        persistjob = true;
>+        if (baseSource) {
>+            nodebase = baseSource->nodeformat;
>+            if (!backingPath &&
>+                !(backingPath = qemuBlockGetBackingStoreString(baseSource)))
>+                goto endjob;
>+        }
>+        device = disk->src->nodeformat;
>+    } else {
>+        device = job->name;
>+    }
>+
>     qemuDomainObjEnterMonitor(driver, vm);
>-    if (baseSource)
>+    if (baseSource && !jobname)

if (!blockdev && baseSource)

>         basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
>                                              baseSource);
>-    if (!baseSource || basePath)

Oh, this is a clever way to avoiding a 'exit_monitor' label and jump to
it when the above command fails.

>-        ret = qemuMonitorBlockStream(priv->mon, device, NULL, false, basePath,
>-                                     NULL, backingPath, speed);
>+
>+    if (!baseSource || basePath || jobname)

Please either:
  * separate the conditions:
    if (blockdev ||
        (the old one || ))
    grab a pint and hide in Winchester until we can get rid of the
    cleverness
  * introduce the exit_monitor label and avoid the need to extend the
    condition at all

>+        ret = qemuMonitorBlockStream(priv->mon, device, jobname, persistjob, basePath,
>+                                     nodebase, backingPath, speed);
>     if (qemuDomainObjExitMonitor(driver, vm) < 0)
>         ret = -1;
>

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190726/8552de9d/attachment-0001.sig>


More information about the libvir-list mailing list