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

Re: [libvirt] [PATCHv5 08/23] blockjob: add qemu capabilities related to block jobs



On 04/17/2012 01:05 AM, Eric Blake wrote:
> The new block copy storage migration sequence requires both the
> 'drive-mirror' and 'drive-reopen' monitor commands, which have
> been proposed[1] for qemu 1.1.  Someday (probably for qemu 1.2),
> these commands may also be added to the 'transaction' monitor
> command for even more power, but we don't need to worry about
> that now.
>
> [1]https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01630.html
>
> * src/qemu/qemu_capabilities.h (QEMU_CAPS_DRIVE_MIRROR)
> (QEMU_CAPS_DRIVE_REOPEN): New bits.
> * src/qemu/qemu_capabilities.c (qemuCaps): Name them.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONCheckCommands): Set
> them.
> (qemuMonitorJSONDiskSnapshot): Fix formatting issues.
> (qemuMonitorJSONDriveMirror, qemuMonitorDriveReopen): New functions.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror)
> (qemuMonitorDriveReopen): Declare them.
> * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror)
> (qemuMonitorDriveReopen): New passthroughs.
> * src/qemu/qemu_monitor.h (qemuMonitorDriveMirror)
> (qemuMonitorDriveReopen): Declare them.
> ---
>
> merge 1/18 and 9/18 from v4
> v5: adjust to latest upstream qemu proposal, which requires 'full'
> argument and no longer permits interaction with 'transaction'
>
>  src/qemu/qemu_capabilities.c |    2 +
>  src/qemu/qemu_capabilities.h |    2 +
>  src/qemu/qemu_monitor.c      |   49 ++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor.h      |   11 +++++++
>  src/qemu/qemu_monitor_json.c |   63 ++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor_json.h |   18 ++++++++++-
>  6 files changed, 143 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index fd4ca4d..0e5dd68 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -159,6 +159,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>
>                "block-job-sync", /* 90 */
>                "block-job-async",
> +              "drive-mirror",
> +              "drive-reopen",
>      );
>
>  struct qemu_feature_flags {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 6550d59..e6fa519 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -126,6 +126,8 @@ enum qemuCapsFlags {
>      QEMU_CAPS_TRANSACTION        = 89, /* transaction monitor command */
>      QEMU_CAPS_BLOCKJOB_SYNC      = 90, /* RHEL 6.2 block_job_cancel */
>      QEMU_CAPS_BLOCKJOB_ASYNC     = 91, /* qemu 1.1 block-job-cancel */
> +    QEMU_CAPS_DRIVE_MIRROR       = 92, /* drive-mirror monitor command */
> +    QEMU_CAPS_DRIVE_REOPEN       = 93, /* drive-reopen monitor command */
>
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 2f66c46..34c7926 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2685,6 +2685,31 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
>      return ret;
>  }
>
> +/* Add the drive-mirror action to a transaction.  */


Is this comment now incorrect? (You say the new qemu proposal doesn't
allow interaction with "transaction")


> +int
> +qemuMonitorDriveMirror(qemuMonitorPtr mon,
> +                       const char *device, const char *file,
> +                       const char *format, unsigned int flags)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, flags=%x",
> +              mon, device, file, format, flags);

Should we be concerned about NULL string pointers for args that aren't
qualified as ATTRIBUTE_NONNULL?

> +
> +    if (!mon) {


I had thought that declaring an arg ATTRIBUTE_NONNULL meant that you
didn't need to check for NULL in the code, so when I saw this, I was
confused and decided to investigate. I found the following gcc bug:

   http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308

conveniently with a comment posted by on "Eric Blake" (any relation? :-)

So does this mean that all of our ATTRIBUTE_NONNULL declarations are
pointless?


> +        qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> +                        _("monitor must not be NULL"));
> +        return -1;
> +    }
> +
> +    if (mon->json)
> +        ret = qemuMonitorJSONDriveMirror(mon, device, file, format, flags);
> +    else
> +        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                        _("drive-mirror requires JSON monitor"));
> +    return ret;
> +}
> +
>  /* Use the transaction QMP command to run atomic snapshot commands.  */
>  int
>  qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
> @@ -2701,6 +2726,30 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
>      return ret;
>  }
>
> +/* Use the drive-reopen monitor command.  */
> +int
> +qemuMonitorDriveReopen(qemuMonitorPtr mon, const char *device,
> +                       const char *file, const char *format)
> +{
> +    int ret = -1;
> +
> +    VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s",
> +              mon, device, file, format);
> +
> +    if (!mon) {
> +        qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> +                        _("monitor must not be NULL"));
> +        return -1;
> +    }
> +
> +    if (mon->json)
> +        ret = qemuMonitorJSONDriveReopen(mon, device, file, format);
> +    else
> +        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                        _("drive-reopen requires JSON monitor"));
> +    return ret;
> +}
> +
>  int qemuMonitorArbitraryCommand(qemuMonitorPtr mon,
>                                  const char *cmd,
>                                  char **reply,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index f3cdcdd..1b4b130 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -510,6 +510,17 @@ int qemuMonitorDiskSnapshot(qemuMonitorPtr mon,
>                              bool reuse);
>  int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +int qemuMonitorDriveMirror(qemuMonitorPtr mon,
> +                           const char *device,
> +                           const char *file,
> +                           const char *format,
> +                           unsigned int flags)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +int qemuMonitorDriveReopen(qemuMonitorPtr mon,
> +                           const char *device,
> +                           const char *file,
> +                           const char *format)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>
>  int qemuMonitorArbitraryCommand(qemuMonitorPtr mon,
>                                  const char *cmd,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index eb58f13..e0ea505 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -987,6 +987,10 @@ qemuMonitorJSONCheckCommands(qemuMonitorPtr mon,
>              qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC);
>          else if (STREQ(name, "block-job-cancel"))
>              qemuCapsSet(qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC);
> +        else if (STREQ(name, "drive-mirror"))
> +            qemuCapsSet(qemuCaps, QEMU_CAPS_DRIVE_MIRROR);
> +        else if (STREQ(name, "drive-reopen"))
> +            qemuCapsSet(qemuCaps, QEMU_CAPS_DRIVE_REOPEN);
>      }
>
>      ret = 0;
> @@ -3199,6 +3203,38 @@ cleanup:
>  }
>
>  int
> +qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
> +                           const char *device, const char *file,
> +                           const char *format, unsigned int flags)
> +{
> +    int ret = -1;
> +    virJSONValuePtr cmd;
> +    virJSONValuePtr reply = NULL;
> +    bool shallow = (flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) != 0;
> +    bool reuse = (flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) != 0;
> +
> +    cmd = qemuMonitorJSONMakeCommand("drive-mirror",
> +                                     "s:device", device,
> +                                     "s:target", file,
> +                                     "b:full", !shallow,
> +                                     "s:mode",
> +                                     reuse ? "existing" : "absolute-paths",
> +                                     format ? "s:format" : NULL, format,
> +                                     NULL);
> +    if (!cmd)
> +        return -1;
> +
> +    if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
> +        goto cleanup;
> +    ret = qemuMonitorJSONCheckError(cmd, reply);
> +
> +cleanup:
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
> +}
> +
> +int
>  qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
>  {
>      int ret = -1;
> @@ -3226,6 +3262,33 @@ cleanup:
>      return ret;
>  }
>
> +int
> +qemuMonitorJSONDriveReopen(qemuMonitorPtr mon,  const char *device,
> +                           const char *file, const char *format)
> +{
> +    int ret;
> +    virJSONValuePtr cmd;
> +    virJSONValuePtr reply = NULL;
> +
> +    cmd = qemuMonitorJSONMakeCommand("drive-reopen",
> +                                     "s:device", device,
> +                                     "s:new-image-file", file,
> +                                     format ? "s:format" : NULL, format,
> +                                     NULL);
> +    if (!cmd)
> +        return -1;
> +
> +    if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
> +        goto cleanup;
> +
> +    ret = qemuMonitorJSONCheckError(cmd, reply);
> +
> +cleanup:
> +    virJSONValueFree(cmd);
> +    virJSONValueFree(reply);
> +    return ret;
> +}
> +
>  int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon,
>                                      const char *cmd_str,
>                                      char **reply_str,
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index aacbb5f..d93c37d 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -230,8 +230,22 @@ int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon,
>                                  const char *device,
>                                  const char *file,
>                                  const char *format,
> -                                bool reuse);
> -int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions);
> +                                bool reuse)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
> +    ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
> +int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
> +                               const char *device,
> +                               const char *file,
> +                               const char *format,
> +                               unsigned int flags)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> +int qemuMonitorJSONDriveReopen(qemuMonitorPtr mon,
> +                               const char *device,
> +                               const char *file,
> +                               const char *format)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>
>  int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon,
>                                      const char *cmd_str,

I'll avoid using the "A word" because I don't know the status of qemu
upstream, and don't want to confuse patchchecker. It all looks okay to
me though, although the ATTRIBUTE_NONNULL stuff was a bit of a
revelation (probably I'm the only one who didn't already know about it...).


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