4:02 a.m.
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(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)
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;
> >