On Mon, Jul 29, 2019 at 10:40:14AM +0200, Peter Krempa wrote:
On Fri, Jul 26, 2019 at 14:12:57 +0200, Ján Tomko wrote:
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@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 ...();
There is no 'else' so this format does not make much sense.
+ if (!cfgdisk)
This is the 'else'
+ 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
No I can't. 'cfgbaseparent' is in the chain of images below now 'tmp' and thus the undef would unref everything ...
+ cfgdisk->src->backingStore = cfgbase; + if (cfgbaseparent) + cfgbaseparent->backingStore = NULL;
... this inhibits deletion of the part of the chain we still want.
+ virObjectUnref(tmp); + } +}
[...]
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
In a void function?
+ 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.
Well I'd have to introduce a function which doesn't do anything here and then populate it later? Should I do it?
Nah. Jano
if (qemuBlockJobRegister(job, vm, disk, false) < 0) return -1;