On 09/27/2016 05:06 AM, Nikolay Shirokovskiy wrote:
>
>
> 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.
True - my concern was more that virDomainBlockJobAbort now has a new
possible error value of -2 that's not described. Still just adding to
the documentation that it can now return -1 or -2 probably isn't the
best idea especially if what you're trying to do is resolve/handle an
issue for a specific case within qemu code.
Other API's within the bowels of qemu (monitor, monitor_json, etc)
returning -2 usually is somehow handled by the caller, but before
returning out into what would be API land would return what the API has
documented ({0, -1}, {true, false}, {NULL, address}, {count, 0, -1}, etc.)
Changing an external API return value is a risky proposition - works for
some instances fails for others... I've seen code that does this:
if (public_api(arg1, arg2) == -1) {
print some error message
return failure
}
even though it's been strongly suggested to do something like
if (public_api(arg1, arg2) < 0) {
get specific error (usually via errno) and perhaps
make a decision based on that... but fall through to
print some error message
return failure
}
>
> 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?
>
I think you need to think about all the callers of
qemuMonitorJSONBlockJobError... I almost hate to use the word fragile
to describe things, but there is a lot of complexity involved with
respect to all the consumers. I just recall hearing about someone doing
a lot of muttering while implementing the code. The snapshot code in
particular (which you seem to have copied to a degree) has been the
source of some fairly "exotic" bug reports.
I personally don't have a lot of insight into the totality of the
blockdev* code. I'd need to spend some time to really understand things
better and provide a more clear/concise answer...
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list