[libvirt] [PATCH 09/11] qemu: blockPull: Refactor the rest of qemuDomainBlockJobImpl

[Peter Krempa]

On Wed, Apr 08, 2015 at 13:10:12 -0400, John Ferlan wrote:
> 
> 
> On 04/01/2015 01:20 PM, Peter Krempa wrote:
> > Since it now handles only block pull code paths we can refactor it and
> > remove tons of cruft.
> > ---
> >  src/qemu/qemu_driver.c       | 86 ++++++++++++++++++++------------------------
> >  src/qemu/qemu_monitor.c      | 30 ++++++++--------
> >  src/qemu/qemu_monitor.h      | 17 ++++-----
> >  src/qemu/qemu_monitor_json.c | 59 +++++++-----------------------
> >  src/qemu/qemu_monitor_json.h | 13 ++++---
> >  5 files changed, 78 insertions(+), 127 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 7b7775d..2dd8ed4 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c

...

> > @@ -16276,15 +16267,15 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> >          basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
> >                                               baseSource);
> >      if (!baseSource || basePath)
> > -        ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
> > -                                  speed, mode, async);
> > +        ret = qemuMonitorBlockStream(priv->mon, device, basePath, backingPath,
> > +                                     speed, modern);
> 
> These don't use your new qemuDomainGetMonitor(vm) - not that it really
> matters, but since you created it...

It was meant for functions that don't need the 'priv' variable otherwise
so that it can be avoided. Here 'priv' is needed for checking a
capability so I did not bother changing it.

> 
> >      if (qemuDomainObjExitMonitor(driver, vm) < 0)
> >          ret = -1;
> > -    if (ret < 0) {
> > +
> > +    if (ret < 0)
> >          goto endjob;
> > -    } else if (mode == BLOCK_JOB_PULL) {
> > -        disk->blockjob = true;
> > -    }
> > +
> > +    disk->blockjob = true;
> > 
> >   endjob:
> >      qemuDomainObjEndJob(driver, vm);

...

> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index 01a4f9a..d02567d 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -4286,57 +4286,24 @@ qemuMonitorJSONBlockJobError(virJSONValuePtr reply,
> > 
> >  /* speed is in bytes/sec */
> >  int
> > -qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
> > -                        const char *device,
> > -                        const char *base,
> > -                        const char *backingName,
> > -                        unsigned long long speed,
> > -                        qemuMonitorBlockJobCmd mode,
> > -                        bool modern)
> > +qemuMonitorJSONBlockStream(qemuMonitorPtr mon,
> > +                           const char *device,
> > +                           const char *base,
> > +                           const char *backingName,
> > +                           unsigned long long speed,
> > +                           bool modern)
> >  {
> >      int ret = -1;
> >      virJSONValuePtr cmd = NULL;
> >      virJSONValuePtr reply = NULL;
> > -    const char *cmd_name = NULL;
> > -
> > -    if (base && (mode != BLOCK_JOB_PULL || !modern)) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("only modern block pull supports base: %s"), base);
> > -        return -1;
> > -    }
> 
> Just checking...
> 
> This change is essentially the same as in qemuDomainBlockPullCommon
> where if (!modern) {} was added right?

Yes. This one would be redundant.

> 
> > -
> > -    if (backingName && mode != BLOCK_JOB_PULL) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > -                       _("backing name is supported only for block pull"));
> > -        return -1;
> > -    }
> 
> And this won't be necessary.... since we no longer have multiple
> (ab)users of the same API

Exactly.

> 
> > -
> > -    if (backingName && !base) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > -                       _("backing name requires a base image"));
> > -        return -1;
> > -    }
> 
> Is there a check for this somewhere that I missed?

The caller ensures that this does not happen. We could leave this one
possibly in if you want.

> 
> > +    const char *cmd_name = modern ? "block-stream" : "block_stream";
> > 
> > -    if (speed && mode == BLOCK_JOB_PULL && !modern) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("only modern block pull supports speed: %llu"),
> > -                       speed);
> > -        return -1;
> > -    }
> 
> And this is the second half of the check in qemuDomainBlockPullCommon
> 
> ACK - in general - Just want to make sure the "if (backingName && !base)
> wasn't erroneously removed.
> 
> John

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150409/8e8ae162/attachment-0001.sig>