[libvirt] [PATCHv4 01/18] blockjob: add qemu capabilities related to block jobs
Laine Stump
laine at laine.org
Tue Apr 10 17:17:44 UTC 2012
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.
More information about the libvir-list
mailing list