[libvirt] [PATCH 2/3] qemu: special error code in case of no job on cancel block job

[John Ferlan]

On 09/07/2016 05:24 AM, Nikolay Shirokovskiy wrote:
> Special error code helps gracefully handle race conditions on
> blockjob cancelling. Consider for example pull backup. We want
> to stop it and in the process we cancel blockjob for some of the
> exported disks. But at the time this command reaches qemu blockjob
> fail for some reason and cancel result and result of stopping
> will be an error with quite unexpecte message - no such job (sort of).
> Instead we can detect this race and treat as correct cancelling
> and wait until fail event will arrive. Then we can report corect
> error message with some details from qemu probably.
> 
> Users of domainBlockJobAbort can be affected as -2 is new possible
> return code. Not sure if I should stick to the original behaviour in
> this case.
> ---
>  src/qemu/qemu_monitor.c      |  5 +++++
>  src/qemu/qemu_monitor_json.c | 11 +++++++----
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 

I haven't looked forward to patch 3/3 yet, but perhaps it'd be better in
the "caller" that cares to :

    if (qemu*() < 0) {
        virErrorPtr err = virGetLastError();
        if (err && err->code == VIR_ERR_OPERATION_INVALID) {
            /* Do something specific */
        }
    }

Returning -2 alters virDomainBlockJobAbort expected return values and
well that's probably not a good idea since we don't know if some caller
has said (if virDomainBlockJobAbort() == -1) instead of <= -1

John

> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 4171914..944856d 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3335,6 +3335,11 @@ qemuMonitorBlockStream(qemuMonitorPtr mon,
>  }
>  
>  
> +/* return:
> + *  0 in case of success
> + * -1 in case of general error
> + * -2 in case there is no such job
> + */
>  int
>  qemuMonitorBlockJobCancel(qemuMonitorPtr mon,
>                            const char *device,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 7903b47..42b05c4 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4351,6 +4351,7 @@ qemuMonitorJSONBlockJobError(virJSONValuePtr reply,
>      if (qemuMonitorJSONErrorIsClass(error, "DeviceNotActive")) {
>          virReportError(VIR_ERR_OPERATION_INVALID,
>                         _("No active operation on device: %s"), device);
> +        return -2;
>      } else if (qemuMonitorJSONErrorIsClass(error, "DeviceInUse")) {
>          virReportError(VIR_ERR_OPERATION_FAILED,
>                         _("Device %s in use"), device);
> @@ -4408,6 +4409,11 @@ qemuMonitorJSONBlockStream(qemuMonitorPtr mon,
>  }
>  
>  
> +/* return:
> + *  0 in case of success
> + * -1 in case of general error
> + * -2 in case there is no such job
> + */
>  int
>  qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon,
>                                const char *device,
> @@ -4426,10 +4432,7 @@ qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon,
>      if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
>          goto cleanup;
>  
> -    if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0)
> -        goto cleanup;
> -
> -    ret = 0;
> +    ret = qemuMonitorJSONBlockJobError(reply, cmd_name, device);
>  
>   cleanup:
>      virJSONValueFree(cmd);
>