Re: [PATCH 8/9] qemu: Add -blockdev support for block pull job

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux