Re: [Qemu-devel] [PATCH v2] blkdebug: make the fault injection functionality callable from QMP

[Eric Blake]

On 08/26/2014 11:34 PM, Hitoshi Mitake wrote:

>>> {"execute": "blkdebug-set-rules", "arguments": {"device": "ide0-hd0",
>>> "rules":[{"event": "write_aio", "type": "inject-error", "immediately":
>>
>> Why 'write_aoi'? New QMP commands should prefer dashes (write-aio) if
>> there is no compelling reason for underscore.
> 
> The name of event is defined in the event_names array of
> block/blkdebug.c, which uses underscore. I think using the original
> names would be good because the name shouldn't be different from
> config file notation of blkdebug.

But it should be fairly easy to code things to accept both spellings
when parsing the config file, while preferring the dash in QAPI.
Furthermore, by defining the enum in QAPI instead of in blkdebug.c, you
get a generated enum that blkdebug.c can reuse, and where you are then
guaranteed that things stay in sync if new commands are added (by adding
them to the .json file).


>>
>>> +    } else if (!strcmp(type, "inject-error")) {
>>> +        int _errno, sector;
>>
>> The name _errno threw me; is there something better without a leading
>> underscore that would work better?
> 
> I added underscore to the name because errno is already used by
> errno.h. How about "given_errno"?

If you use proper .json typing, the generator will automatically rename
the QMP interface "errno" to the C code "q_errno".  Consistently using
the type-safe munging already done by the generator, instead of redoing
a completely different munging on your own, is the preferred solution here.


>>> +
>>> +        once = qdict_get_try_bool(dict, "once", 0);
>>
>> s/0/bool/ - we use <stdbool.h>, so you should use the named constants
>> when dealing with bool parameters.
> 
> Thanks, I'll fix it in v3.

Actually, if you use a type-safe qapi definition, you may not even need
to do raw qdict operations, but can just directly use the C struct that
gets generated as a result of the qapi.


> 
> I also like the detailed specification. But, there are bunch of event
> names (the event_names array of block/blkdebug.c). In addition, the
> rule of blkdebug can be extended. So I think defining it in two palces
> (qapi-schema.json and block/blkdebug.c) is hard to maintain. Parsing
> qdict in blkdebug.c would be simpler. How do you think?

No, don't do duplication.  Instead, fix blkdebug.c to USE the enum
generated by the .json, and you only have to maintain the list in one
place - the .json file.

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

signature.asc
Description: OpenPGP digital signature