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

[Nikolay Shirokovskiy]

On 26.09.2016 23:07, John Ferlan wrote:
> 
> 
> 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
> 

It is common place in libvirt to have special error codes instead of 0/-1
only (qemuMonitorJSONCheckCPUx86) and it feels bulky to have checks that
involve virGetLastError. It looks like other places that use similar 
construction do not have other choice because of rpc call or smth.

I think when function spawns error when it comes to error code
value is does not take much care as they are not really semantic
(at least such common error as invalid operation) 
so one day the caller can catch 'operation invalid' for different
reason from different place on the stack.

As to virDomainBlockJobAbort it is easy to patch)

I think I should also remove reporting error from qemuMonitorJSONBlockJobError 
in case of DeviceNotActive however it is used from several places even
like setting speed. It is legacy of commit b976165c when
a lot of qemu commands share common error checking code. I think I'd
better copy this code to qemuMonitorJSONBlockJobCancel and change it there
instead of common place. What do you think?

Nikolay