[libvirt] [PATCH 0/8] qemu: monitor: Clean up error handling for block job APIs
Remove handling of errors which QEMU no longer reports and rename fields so that it conforms with the new semantics. Peter Krempa (8): qemu: monitor: Remove error classes not conforming to QAPI schema qemu: monitor: Use qemuMonitorJSONCheckError in qemuMonitorJSONBlockJobError qemu: monitor: Remove temporary variables qemu: monitor: Use qemuMonitorJSONCheckError in qemuMonitorJSONBlockStream qemu: monitor: Move qemuMonitorJSONDrivePivot together with block-job APIs qemu: monitor: Use qemuMonitorJSONBlockJobError in qemuMonitorJSONDrivePivot qemu: monitor: Rename 'device' argument for block job control APIs qemu: monitor: Separate probing for active block commit src/qemu/qemu_monitor.c | 20 +++--- src/qemu/qemu_monitor.h | 6 +- src/qemu/qemu_monitor_json.c | 155 +++++++++++++++++++++---------------------- src/qemu/qemu_monitor_json.h | 9 ++- 4 files changed, 94 insertions(+), 96 deletions(-) -- 2.16.2
Both were removed prior to qemu v1.2.0-rc0 when switching to the new error format where almost all error types were converted to GenericError. Relevant qemu commits are <de253f14912e> and <df1e608a01eb0> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 2921f110a9..7bb3e85229 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4670,12 +4670,6 @@ qemuMonitorJSONBlockJobError(virJSONValuePtr reply, if (qemuMonitorJSONErrorIsClass(error, "DeviceNotActive")) { virReportError(VIR_ERR_OPERATION_INVALID, _("No active operation on device: %s"), device); - } else if (qemuMonitorJSONErrorIsClass(error, "DeviceInUse")) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Device %s in use"), device); - } else if (qemuMonitorJSONErrorIsClass(error, "NotSupported")) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Operation is not supported for device: %s"), device); } else if (qemuMonitorJSONErrorIsClass(error, "CommandNotFound")) { virReportError(VIR_ERR_OPERATION_INVALID, _("Command '%s' is not found"), cmd_name); -- 2.16.2
On 08/15/2018 07:52 AM, Peter Krempa wrote:
Both were removed prior to qemu v1.2.0-rc0 when switching to the new error format where almost all error types were converted to GenericError.
Relevant qemu commits are <de253f14912e> and <df1e608a01eb0>
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 6 ------ 1 file changed, 6 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Report the generic errors using the existing function so that we don't reimplement the same functionality multiple times. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7bb3e85229..a361a0acd9 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4658,29 +4658,20 @@ qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon) static int -qemuMonitorJSONBlockJobError(virJSONValuePtr reply, - const char *cmd_name, +qemuMonitorJSONBlockJobError(virJSONValuePtr cmd, + virJSONValuePtr reply, const char *device) { virJSONValuePtr error; - if (!(error = virJSONValueObjectGet(reply, "error"))) - return 0; - - if (qemuMonitorJSONErrorIsClass(error, "DeviceNotActive")) { + if ((error = virJSONValueObjectGet(reply, "error")) && + (qemuMonitorJSONErrorIsClass(error, "DeviceNotActive"))) { virReportError(VIR_ERR_OPERATION_INVALID, _("No active operation on device: %s"), device); - } else if (qemuMonitorJSONErrorIsClass(error, "CommandNotFound")) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Command '%s' is not found"), cmd_name); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected error: (%s) '%s'"), - NULLSTR(virJSONValueObjectGetString(error, "class")), - NULLSTR(virJSONValueObjectGetString(error, "desc"))); + return -1; } - return -1; + return qemuMonitorJSONCheckError(cmd, reply); } @@ -4708,7 +4699,7 @@ qemuMonitorJSONBlockStream(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0) + if (qemuMonitorJSONBlockJobError(cmd, reply, device) < 0) goto cleanup; ret = 0; @@ -4737,7 +4728,7 @@ qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0) + if (qemuMonitorJSONBlockJobError(cmd, reply, device) < 0) goto cleanup; ret = 0; @@ -4768,7 +4759,7 @@ qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0) + if (qemuMonitorJSONBlockJobError(cmd, reply, device) < 0) goto cleanup; ret = 0; -- 2.16.2
On 08/15/2018 07:52 AM, Peter Krempa wrote:
Report the generic errors using the existing function so that we don't reimplement the same functionality multiple times.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Now that the job name is used in single place in the respective functions remove the temporary strings. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a361a0acd9..931d3dd985 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4686,9 +4686,8 @@ qemuMonitorJSONBlockStream(qemuMonitorPtr mon, int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; - const char *cmd_name = "block-stream"; - if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, + if (!(cmd = qemuMonitorJSONMakeCommand("block-stream", "s:device", device, "Y:speed", speed, "S:base", base, @@ -4718,9 +4717,8 @@ qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; - const char *cmd_name = "block-job-cancel"; - if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, + if (!(cmd = qemuMonitorJSONMakeCommand("block-job-cancel", "s:device", device, NULL))) return -1; @@ -4748,9 +4746,8 @@ qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, int ret = -1; virJSONValuePtr cmd; virJSONValuePtr reply = NULL; - const char *cmd_name = "block-job-set-speed"; - if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, + if (!(cmd = qemuMonitorJSONMakeCommand("block-job-set-speed", "s:device", device, "J:speed", speed, NULL))) -- 2.16.2
On 08/15/2018 07:52 AM, Peter Krempa wrote:
Now that the job name is used in single place in the respective functions remove the temporary strings.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The API does not report any special job-related error so the generic error function should be used. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 931d3dd985..a1c0d5fc15 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4698,7 +4698,7 @@ qemuMonitorJSONBlockStream(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONBlockJobError(cmd, reply, device) < 0) + if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; ret = 0; -- 2.16.2
On 08/15/2018 07:52 AM, Peter Krempa wrote:
The API does not report any special job-related error so the generic error function should be used.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Move all relevant APIs dealing with existing jobs together. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 55 ++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a1c0d5fc15..fed2f50e28 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4288,33 +4288,6 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, return ret; } -int -qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, - const char *device) -{ - int ret = -1; - virJSONValuePtr cmd; - virJSONValuePtr reply = NULL; - - cmd = qemuMonitorJSONMakeCommand("block-job-complete", - "s:device", device, - NULL); - if (!cmd) - return -1; - - if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; - - if (qemuMonitorJSONCheckError(cmd, reply) < 0) - goto cleanup; - - ret = 0; - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; -} - static char * qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image, @@ -4768,6 +4741,34 @@ qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, } +int +qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, + const char *device) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("block-job-complete", + "s:device", device, + NULL); + if (!cmd) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + ret = 0; + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon, const char *protocol, const char *fdname, -- 2.16.2
On 08/15/2018 07:52 AM, Peter Krempa wrote:
Move all relevant APIs dealing with existing jobs together.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 55 ++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 27 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The API deals with a block job so use the common error reporting function for it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index fed2f50e28..670089b87c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4758,7 +4758,7 @@ qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONCheckError(cmd, reply) < 0) + if (qemuMonitorJSONBlockJobError(cmd, reply, device) < 0) goto cleanup; ret = 0; -- 2.16.2
On 08/15/2018 07:52 AM, Peter Krempa wrote:
The API deals with a block job so use the common error reporting function for it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Very strange - this got applied to qemuMonitorJSONBlockStream in my "git am" of the patches... Gets stranger on the next patch too, but at least this explains what happened there... Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Thu, Aug 23, 2018 at 17:38:39 -0400, John Ferlan wrote:
On 08/15/2018 07:52 AM, Peter Krempa wrote:
The API deals with a block job so use the common error reporting function for it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Very strange - this got applied to qemuMonitorJSONBlockStream in my "git am" of the patches... Gets stranger on the next patch too, but at least this explains what happened there...
Yeah. The blockdev series probably shifted some code arround (I was adding the APIs for media manipulation) and the context was not big enough to apply this correctly. I had the same problem now when I've rebased it locally. I've made sure now that it's applied at the correct point.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Starting from qemu 2.7 the 'device' argument is in fact a name of the job itself. Change our APIs accordingly and adjust the error message. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 18 +++++++++--------- src/qemu/qemu_monitor.h | 6 +++--- src/qemu/qemu_monitor_json.c | 22 +++++++++++----------- src/qemu/qemu_monitor_json.h | 6 +++--- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6e0644221b..9bc7aa9ed1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3306,13 +3306,13 @@ qemuMonitorDiskNameLookup(qemuMonitorPtr mon, /* Use the block-job-complete monitor command to pivot a block copy job. */ int qemuMonitorDrivePivot(qemuMonitorPtr mon, - const char *device) + const char *jobname) { - VIR_DEBUG("device=%s", device); + VIR_DEBUG("jobname=%s", jobname); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONDrivePivot(mon, device); + return qemuMonitorJSONDrivePivot(mon, jobname); } @@ -3386,26 +3386,26 @@ qemuMonitorBlockStream(qemuMonitorPtr mon, int qemuMonitorBlockJobCancel(qemuMonitorPtr mon, - const char *device) + const char *jobname) { - VIR_DEBUG("device=%s", device); + VIR_DEBUG("jobname=%s", jobname); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONBlockJobCancel(mon, device); + return qemuMonitorJSONBlockJobCancel(mon, jobname); } int qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, - const char *device, + const char *jobname, unsigned long long bandwidth) { - VIR_DEBUG("device=%s, bandwidth=%lluB", device, bandwidth); + VIR_DEBUG("jobname=%s, bandwidth=%lluB", jobname, bandwidth); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONBlockJobSetSpeed(mon, device, bandwidth); + return qemuMonitorJSONBlockJobSetSpeed(mon, jobname, bandwidth); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2fa8d5b51d..ac41827693 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -861,7 +861,7 @@ int qemuMonitorBlockdevMirror(qemuMonitorPtr mon, unsigned int flags) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); int qemuMonitorDrivePivot(qemuMonitorPtr mon, - const char *device) + const char *jobname) ATTRIBUTE_NONNULL(2); int qemuMonitorBlockCommit(qemuMonitorPtr mon, @@ -903,11 +903,11 @@ int qemuMonitorBlockStream(qemuMonitorPtr mon, ATTRIBUTE_NONNULL(2); int qemuMonitorBlockJobCancel(qemuMonitorPtr mon, - const char *device) + const char *jobname) ATTRIBUTE_NONNULL(2); int qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, - const char *device, + const char *jobname, unsigned long long bandwidth); typedef struct _qemuMonitorBlockJobInfo qemuMonitorBlockJobInfo; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 670089b87c..51b1fddccf 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4633,14 +4633,14 @@ qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon) static int qemuMonitorJSONBlockJobError(virJSONValuePtr cmd, virJSONValuePtr reply, - const char *device) + const char *jobname) { virJSONValuePtr error; if ((error = virJSONValueObjectGet(reply, "error")) && (qemuMonitorJSONErrorIsClass(error, "DeviceNotActive"))) { virReportError(VIR_ERR_OPERATION_INVALID, - _("No active operation on device: %s"), device); + _("No active block job '%s'"), jobname); return -1; } @@ -4685,21 +4685,21 @@ qemuMonitorJSONBlockStream(qemuMonitorPtr mon, int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, - const char *device) + const char *jobname) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; if (!(cmd = qemuMonitorJSONMakeCommand("block-job-cancel", - "s:device", device, + "s:device", jobname, NULL))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONBlockJobError(cmd, reply, device) < 0) + if (qemuMonitorJSONBlockJobError(cmd, reply, jobname) < 0) goto cleanup; ret = 0; @@ -4713,7 +4713,7 @@ qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, - const char *device, + const char *jobname, unsigned long long speed) { int ret = -1; @@ -4721,7 +4721,7 @@ qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, virJSONValuePtr reply = NULL; if (!(cmd = qemuMonitorJSONMakeCommand("block-job-set-speed", - "s:device", device, + "s:device", jobname, "J:speed", speed, NULL))) return -1; @@ -4729,7 +4729,7 @@ qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONBlockJobError(cmd, reply, device) < 0) + if (qemuMonitorJSONBlockJobError(cmd, reply, jobname) < 0) goto cleanup; ret = 0; @@ -4743,14 +4743,14 @@ qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, - const char *device) + const char *jobname) { int ret = -1; virJSONValuePtr cmd; virJSONValuePtr reply = NULL; cmd = qemuMonitorJSONMakeCommand("block-job-complete", - "s:device", device, + "s:device", jobname, NULL); if (!cmd) return -1; @@ -4758,7 +4758,7 @@ qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (qemuMonitorJSONBlockJobError(cmd, reply, device) < 0) + if (qemuMonitorJSONBlockJobError(cmd, reply, jobname) < 0) goto cleanup; ret = 0; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 2408ab0c5b..013bee3364 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -264,7 +264,7 @@ int qemuMonitorJSONBlockdevMirror(qemuMonitorPtr mon, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, - const char *device) + const char *jobname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, @@ -307,11 +307,11 @@ int qemuMonitorJSONBlockStream(qemuMonitorPtr mon, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, - const char *device) + const char *jobname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, - const char *device, + const char *jobname, unsigned long long speed) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 2.16.2
On 08/15/2018 07:52 AM, Peter Krempa wrote:
Starting from qemu 2.7 the 'device' argument is in fact a name of the job itself. Change our APIs accordingly and adjust the error message.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 18 +++++++++--------- src/qemu/qemu_monitor.h | 6 +++--- src/qemu/qemu_monitor_json.c | 22 +++++++++++----------- src/qemu/qemu_monitor_json.h | 6 +++--- 4 files changed, 26 insertions(+), 26 deletions(-)
Really, bizarre... The 'git am' of this series changed the qemuMonitorJSONBlockJobError in qemuMonitorJSONBlockStream to pass @jobname instead of @device, *BUT* that is not changed here. Very, very, odd git am behaviour. Too many closely named functions too close together is my guess for the pattern match. What's in the patch is fine, just was confusing when my build failed! Reviewed-by: John Ferlan <jferlan@redhat.com> John
Extract the code used to probe for the functionality so that it does not litter the code used for actual work. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor_json.c | 58 ++++++++++++++++++++++++++------------------ src/qemu/qemu_monitor_json.h | 3 +++ 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 9bc7aa9ed1..a60e78d967 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3285,7 +3285,7 @@ qemuMonitorSupportsActiveCommit(qemuMonitorPtr mon) if (!mon || !mon->json) return false; - return qemuMonitorJSONBlockCommit(mon, "bogus", NULL, NULL, NULL, 0) == -2; + return qemuMonitorJSONSupportsActiveCommit(mon); } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 51b1fddccf..68ef9b3ae4 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4231,14 +4231,42 @@ qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr *actions) return ret; } +/* Probe if active commit is supported: pass in a bogus device and NULL top + * and base. The probe return is true if active commit is detected or false + * if not supported or on any error */ +bool +qemuMonitorJSONSupportsActiveCommit(qemuMonitorPtr mon) +{ + bool ret = false; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("block-commit", "s:device", + "bogus", NULL))) + return false; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) { + VIR_DEBUG("block-commit supports active commit"); + ret = true; + goto cleanup; + } + + /* This is a false negative for qemu 2.0; but probably not + * worth the additional complexity to worry about it */ + VIR_DEBUG("block-commit requires 'top' parameter, " + "assuming it lacks active commit"); + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + /* speed is in bytes/sec. Returns 0 on success, -1 with error message - * emitted on failure. - * - * Additionally, can be used to probe if active commit is supported: - * pass in a bogus device and NULL top and base. The probe return is - * -2 if active commit is detected, -3 if inconclusive; with no error - * message issued. - */ + * emitted on failure. */ int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, const char *base, @@ -4261,22 +4289,6 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - if (!top && !base) { - /* Normally we always specify top and base; but omitting them - * allows for probing whether qemu is new enough to support - * live commit. */ - if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) { - VIR_DEBUG("block-commit supports active commit"); - ret = -2; - } else { - /* This is a false negative for qemu 2.0; but probably not - * worth the additional complexity to worry about it */ - VIR_DEBUG("block-commit requires 'top' parameter, " - "assuming it lacks active commit"); - ret = -3; - } - goto cleanup; - } if (qemuMonitorJSONCheckError(cmd, reply) < 0) goto cleanup; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 013bee3364..6930ef34c4 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -267,6 +267,9 @@ int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, const char *jobname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +bool qemuMonitorJSONSupportsActiveCommit(qemuMonitorPtr mon) + ATTRIBUTE_NONNULL(1); + int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, const char *top, -- 2.16.2
On 08/15/2018 07:52 AM, Peter Krempa wrote:
Extract the code used to probe for the functionality so that it does not litter the code used for actual work.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor_json.c | 58 ++++++++++++++++++++++++++------------------ src/qemu/qemu_monitor_json.h | 3 +++ 3 files changed, 39 insertions(+), 24 deletions(-)
This one somehow feels related to the other series I reviewed where the *top was or wasn't present to determine whether capability was there, but I see that it'd still be useful - just the irony of it all. Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 9bc7aa9ed1..a60e78d967 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c
NB: My gitk view shows a few lines above here the call to qemuMonitorJSONBlockCommit has a second row of args that are not aligned properly... Trivialities. [...]
On Thu, Aug 23, 2018 at 18:11:09 -0400, John Ferlan wrote:
On 08/15/2018 07:52 AM, Peter Krempa wrote:
Extract the code used to probe for the functionality so that it does not litter the code used for actual work.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor_json.c | 58 ++++++++++++++++++++++++++------------------ src/qemu/qemu_monitor_json.h | 3 +++ 3 files changed, 39 insertions(+), 24 deletions(-)
This one somehow feels related to the other series I reviewed where the *top was or wasn't present to determine whether capability was there, but I see that it'd still be useful - just the irony of it all.
Your feelings are spot on. I was doing this commit when I realized that the detection is pointless at this point and cleaned it up right away so that we don't have to carry it around forever. The removal of the detection is orthogonal to this cleanup though and since we can't delete the detection code completely at this point thus the other patches were sent in a separate series.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 9bc7aa9ed1..a60e78d967 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c
NB: My gitk view shows a few lines above here the call to qemuMonitorJSONBlockCommit has a second row of args that are not aligned properly... Trivialities.
That will be fixed later. I'm adding few new args for block-commit. That was not published yet though.
On Wed, Aug 15, 2018 at 13:52:51 +0200, Peter Krempa wrote:
Remove handling of errors which QEMU no longer reports and rename fields so that it conforms with the new semantics.
Peter Krempa (8): qemu: monitor: Remove error classes not conforming to QAPI schema qemu: monitor: Use qemuMonitorJSONCheckError in qemuMonitorJSONBlockJobError qemu: monitor: Remove temporary variables qemu: monitor: Use qemuMonitorJSONCheckError in qemuMonitorJSONBlockStream qemu: monitor: Move qemuMonitorJSONDrivePivot together with block-job APIs qemu: monitor: Use qemuMonitorJSONBlockJobError in qemuMonitorJSONDrivePivot qemu: monitor: Rename 'device' argument for block job control APIs qemu: monitor: Separate probing for active block commit
src/qemu/qemu_monitor.c | 20 +++--- src/qemu/qemu_monitor.h | 6 +- src/qemu/qemu_monitor_json.c | 155 +++++++++++++++++++++---------------------- src/qemu/qemu_monitor_json.h | 9 ++- 4 files changed, 94 insertions(+), 96 deletions(-)
Ping?
participants (2)
-
John Ferlan -
Peter Krempa