[libvirt] [PATCH 8/9] qemu: Add -blockdev support for block pull job
Ján Tomko
jtomko at redhat.com
Mon Jul 29 09:02:00 UTC 2019
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 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
>> ...();
>
>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;
>> >
-------------- 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/20190729/1389130a/attachment-0001.sig>
More information about the libvir-list
mailing list