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@xxxxxxxxxx>
> > ---
> > 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;
> >
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
