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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]