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

Re: [libvirt] [PATCHv4 01/18] blockjob: add qemu capabilities related to block jobs



On 04/09/2012 11:52 PM, Eric Blake wrote:
> The new block copy storage migration sequence requires both the
> 'drive-mirror' action in 'transaction' (present if the 'drive-mirror'
> standalone monitor command also exists) and the 'drive-reopen' monitor
> command (it would be nice if that were also part of a 'transaction',
> but the initial qemu implementation has it standalone only).
>
> Furthermore, qemu 1.1 will be adding 'block_job_cancel' as an
> asynchronous command, but an earlier implementation (that supported
> only the qed file format) existed which was synchronous only.  If
> 'drive-mirror' is added in time, then we can safely use 'drive-mirror'
> as the witness of whether 'block_job_cancel' is synchronous or
> asynchronous.
>
> As of this[1] qemu email, both commands have been proposed but not yet
> incorporated into the tree; in fact, the implementation I tested with
> has changed to match this[2] email that suggested a mandatory
> 'full':'bool' argument rather than 'mode':'no-backing-file'.  So there
> is a risk that qemu 1.1 will have yet another subtly different
> implementation.
> [1]https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg01524.html
> [2]https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg00886.html


None of this gives me warm fuzzies :-/


>
> * 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.
> ---
>  src/qemu/qemu_capabilities.c |    3 +++
>  src/qemu/qemu_capabilities.h |    2 ++
>  src/qemu/qemu_monitor_json.c |   15 ++++++++-------
>  3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 0e09d6d..1938ae4 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -156,6 +156,9 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>                "scsi-disk.channel",
>                "scsi-block",
>                "transaction",
> +
> +              "drive-mirror", /* 90 */
> +              "drive-reopen",
>      );
>
>  struct qemu_feature_flags {
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 78cdbe0..405bf2a 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -124,6 +124,8 @@ enum qemuCapsFlags {
>      QEMU_CAPS_SCSI_DISK_CHANNEL  = 87, /* Is scsi-disk.channel available? */
>      QEMU_CAPS_SCSI_BLOCK         = 88, /* -device scsi-block */
>      QEMU_CAPS_TRANSACTION        = 89, /* transaction monitor command */
> +    QEMU_CAPS_DRIVE_MIRROR       = 90, /* drive-mirror monitor command */
> +    QEMU_CAPS_DRIVE_REOPEN       = 91, /* drive-reopen monitor command */
>
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index d09d779..c8ed087 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -939,12 +939,14 @@ qemuMonitorJSONCheckCommands(qemuMonitorPtr mon,
>
>          if (STREQ(name, "human-monitor-command"))
>              *json_hmp = 1;
> -
> -        if (STREQ(name, "system_wakeup"))
> +        else if (STREQ(name, "system_wakeup"))
>              qemuCapsSet(qemuCaps, QEMU_CAPS_WAKEUP);
> -
> -        if (STREQ(name, "transaction"))
> +        else if (STREQ(name, "transaction"))
>              qemuCapsSet(qemuCaps, QEMU_CAPS_TRANSACTION);
> +        else if (STREQ(name, "drive-mirror"))
> +            qemuCapsSet(qemuCaps, QEMU_CAPS_DRIVE_MIRROR);
> +        else if (STREQ(name, "drive-reopen"))
> +            qemuCapsSet(qemuCaps, QEMU_CAPS_DRIVE_REOPEN);


Good optimization (turning the if's into else if's). I notice qemu is
inconsistent wrt "-" vs. "_" in command names (yeah, I'm in a snarky
mood ;-))


>      }
>
>      ret = 0;
> @@ -3114,7 +3116,7 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
>                              const char *device, const char *file,
>                              const char *format, bool reuse)
>  {
> -    int ret;
> +    int ret = -1;
>      virJSONValuePtr cmd;
>      virJSONValuePtr reply = NULL;
>
> @@ -3132,14 +3134,13 @@ qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
>      if (actions) {
>          if (virJSONValueArrayAppend(actions, cmd) < 0) {
>              virReportOOMError();
> -            ret = -1;
>          } else {
>              ret = 0;
>              cmd = NULL;
>          }
>      } else {
>          if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
> -        goto cleanup;
> +            goto cleanup;
>
>          if (qemuMonitorJSONHasError(reply, "CommandNotFound") &&
>              qemuMonitorCheckHMP(mon, "snapshot_blkdev")) {

I guess you just snuck this in here on general principle, right? Since
there's no functional change, and it's rather minor, I don't have a
problem including it.

ACK.


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