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