[libvirt] [PATCH v3 04/18] blockjob: split out block info monitor handling

[Peter Krempa]

On 08/31/14 06:02, Eric Blake wrote:
> Another layer of overly-multiplexed code that deserves to be
> split into obviously separate paths for query vs. modify.
> This continues the cleanup started in the previous patch.
> 
> In the process, make some tweaks to simplify the logic when
> parsing the JSON reply.  There should be no user-visible
> semantic changes.
> 
> * src/qemu/qemu_monitor.h (qemuMonitorBlockJob): Drop parameter.
> (qemuMonitorBlockJobInfo): New prototype.
> (BLOCK_JOB_INFO): Drop enum.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONBlockJob)
> (qemuMonitorJSONBlockJobInfo): Likewise.
> * src/qemu/qemu_monitor.c (qemuMonitorBlockJob): Split...
> (qemuMonitorBlockJobInfo): ...into second function.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONBlockJob): Move
> block info portions...
> (qemuMonitorJSONGetBlockJobInfo): ...here, and rename...
> (qemuMonitorJSONBlockJobInfo): ...and export.
> (qemuMonitorJSONGetBlockJobInfoOne): Alter return semantics.
> * src/qemu/qemu_driver.c (qemuDomainBlockPivot)
> (qemuDomainBlockJobImpl, qemuDomainGetBlockJobInfo): Adjust
> callers.
> * src/qemu/qemu_migration.c (qemuMigrationDriveMirror)
> (qemuMigrationCancelDriveMirror): Likewise.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/qemu/qemu_driver.c       | 11 +++-----
>  src/qemu/qemu_migration.c    |  7 +++--
>  src/qemu/qemu_monitor.c      | 41 ++++++++++++++++++++--------
>  src/qemu/qemu_monitor.h      |  7 +++--
>  src/qemu/qemu_monitor_json.c | 63 ++++++++++++++++++++++++--------------------
>  src/qemu/qemu_monitor_json.h |  6 ++++-
>  6 files changed, 81 insertions(+), 54 deletions(-)
> 

...

> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 4fd6f01..947ca34 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -695,11 +694,15 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
>                          const char *base,
>                          const char *backingName,
>                          unsigned long bandwidth,
> -                        virDomainBlockJobInfoPtr info,
>                          qemuMonitorBlockJobCmd mode,
>                          bool modern)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> 
> +int qemuMonitorBlockJobInfo(qemuMonitorPtr mon,
> +                            const char *device,
> +                            virDomainBlockJobInfoPtr info)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

Implementation of this function doesn't check for presence of the @info
structure and dereferences it always. You should mark argument 3 nonnull
too.


> +
>  int qemuMonitorOpenGraphics(qemuMonitorPtr mon,
>                              const char *protocol,
>                              int fd,

...

> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 62e7d5d..68ba084 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c

> @@ -3733,54 +3735,66 @@ static int qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry,
>                         _("entry was missing 'len'"));
>          return -1;
>      }
> -    return 0;
> +    return 1;
>  }
> 
> -/** qemuMonitorJSONGetBlockJobInfo:
> - * Parse Block Job information.
> - * The reply is a JSON array of objects, one per active job.
> +/**
> + * qemuMonitorJSONBlockJobInfo:
> + * Parse Block Job information, and populate info for the named device.
> + * Return 1 if info available, 0 if device has no block job, and -1 on error.
>   */
> -static int qemuMonitorJSONGetBlockJobInfo(virJSONValuePtr reply,
> -                                           const char *device,
> -                                           virDomainBlockJobInfoPtr info)
> +int
> +qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
> +                            const char *device,
> +                            virDomainBlockJobInfoPtr info)
>  {
> +    virJSONValuePtr cmd = NULL;
> +    virJSONValuePtr reply = NULL;
>      virJSONValuePtr data;
>      int nr_results;
>      size_t i;
> +    int ret;
> 
> -    if (!info)
> +    cmd = qemuMonitorJSONMakeCommand("query-block-jobs", NULL);
> +    if (!cmd)
>          return -1;
> +    ret = qemuMonitorJSONCommand(mon, cmd, &reply);
> +    if (ret < 0)
> +        goto cleanup;
> 
>      if ((data = virJSONValueObjectGet(reply, "return")) == NULL) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("reply was missing return data"));
> -        return -1;
> +        goto cleanup;
>      }
> 
>      if (data->type != VIR_JSON_TYPE_ARRAY) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("unrecognized format of block job information"));
> -        return -1;
> +        goto cleanup;
>      }
> 
>      if ((nr_results = virJSONValueArraySize(data)) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("unable to determine array size"));
> -        return -1;
> +        goto cleanup;
>      }
> 
> -    for (i = 0; i < nr_results; i++) {
> +    for (i = 0, ret = 0; i < nr_results && ret == 0; i++) {
>          virJSONValuePtr entry = virJSONValueArrayGet(data, i);
>          if (!entry) {
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                             _("missing array element"));
> -            return -1;
> +            ret = -1;
> +            goto cleanup;
>          }
> -        if (qemuMonitorJSONGetBlockJobInfoOne(entry, device, info) == 0)
> -            return 1;
> +        ret = qemuMonitorJSONGetBlockJobInfoOne(entry, device, info);

This doesn't break the loop, thus if the device info isn't last in the
structure you'd overwrite the return code. I presume the clients don't
check that far but you've documented that semantics.

>      }
> 
> -    return 0;
> + cleanup:
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
>  }
> 
> 

...

> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index d8c9308..d3f6f37 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -285,11 +285,15 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon,
>                              const char *base,
>                              const char *backingName,
>                              unsigned long long speed,
> -                            virDomainBlockJobInfoPtr info,
>                              qemuMonitorBlockJobCmd mode,
>                              bool modern)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> 
> +int qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon,
> +                                const char *device,
> +                                virDomainBlockJobInfoPtr info)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

Again, @info is required, thus should be marked ATTRIBUTE_NONNULL.

> +
>  int qemuMonitorJSONSetLink(qemuMonitorPtr mon,
>                             const char *name,
>                             virDomainNetInterfaceLinkState state);
> 

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140904/bb679b6f/attachment-0001.sig>