Re: [Qemu-devel] [PATCH V19 06/12] quorum: Add quorum mechanism.

[Eric Blake]

On 02/21/2014 03:30 PM, Benoît Canet wrote:

>>> +
>>> +- "ret":          The IO return code.
>>
>> What values is this likely to contain?  Is it a finite set, in which
>> case it would be nice to have a QAPI enum that describes the set of
>> return codes, rather than a raw number?
> 
> It's anything that the block stack could return as an error.

Yes, but returning a raw integer is not friendly.  What do those
integers mean?  In this patch, I found only two callers that set this
parameter:

quorum_report_bad_versions:
+            quorum_report_bad(acb, s->bs[item->index]->node_name, 0);

So the code means nothing there, because you always pass 0.

quroum_aio_cb:
     if (ret == 0) {
         acb->success_count++;
+    } else {
+        quorum_report_bad(acb, sacb->aiocb->bs->node_name, ret);

Here, ret is non-zero - but will it ever be anything other than -1?

Seriously, I think you should change this to NOT be an integer, but
instead be an enum value where the enum is tracked in qapi-schema.json.
 Change the signature of quorum_report_bad to take the enum value,
rather than a raw int.  And over the wire, the QMP event will appear
with a string encoding of the enum name, where we can add named errors
to the enum type as we come up with what different error codes we want
to distinguish.  I'm opposed to the current nebulous "it's whatever the
block stack wanted to report" without knowing what it means.  If you
can't switch to an enum, it may be better to just delete the field
entirely, if you can't justify what it is doing.  QMP-wise, we can
always add more information later, but once the release happens, we
can't take away information, even if later refactoring makes it harder
to support.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature