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 <[email protected]> > > --- > > 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;
> >
signature.asc
Description: PGP signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
