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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list