[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