3:40 a.m.
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(a)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)
> + 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?
> if (qemuBlockJobRegister(job, vm, disk, false) < 0)
> return -1;
>