Re: [Qemu-devel] [PATCH v3 03/12] qtest: Document calling conventions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/28/2017 01:32 PM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> We have two flavors of vararg usage in qtest; make it clear that
>>> qmp() has different semantics than hmp(), and let the compiler
>>> enforce that hmp() is used correctly. However, qmp() (and friends)
>>> only accept a subset of printf flags look-alikes (namely, those
>>> that our JSON parser understands), and what is worse, qmp("true")
>>> (the JSON keyword 'true') is different from qmp("%s", "true")
>>> (the JSON string '"true"'), and we have some intermediate cleanup
>>> patches to do before we can mark those as printf-like.
>> 
>> It's not "worse", it's just different :)
>> 
>> Suggest:
>> 
>>   We have two flavors of vararg usage in qtest: qtest_hmp() etc. work
>>   like sprintf(), and qtest_qmp() etc. work like qobject_from_jsonf().
>>   Spell that out in the comments.
>> 
>>   Also add GCC_FMT_ATTR() to qtest_hmp() etc. so that the compiler can
>>   flag incorrect use.
>> 
>>   We have some cleanup work to do before we can do the same for
>>   qtest_qmp() etc.  This will get us the same better-than-nothing
>>   checking we already have for qobject_from_jsonf(): common incorrect
>>   uses of supported conversion specifications will be flagged
>>   (e.g. passing a double for %d), but use of unsupported ones won't.
>
> "Mikey likes it" (no idea if that pop culture reference from my
> childhood has broader range than the US)

'fraid I'm out of range :)

>>> @@ -134,19 +140,19 @@ QDict *qtest_qmp_eventwait_ref(QTestState *s, const 
>>> char *event);
>>>  /**
>>>   * qtest_hmp:
>>>   * @s: #QTestState instance to operate on.
>>> - * @fmt...: HMP command to send to QEMU
>>> + * @fmt...: HMP command to send to QEMU, formats arguments like vsprintf().
>> 
>> Like sprintf().
>
> Hmm, you asked me to use vsprintf on the last one.  Oh, I finally see -
> you're trying to get me to match: vsprintf if it is 'va_list', sprintf
> if it is '...'.  Yeah, that makes sense.

Correct.  I should've explained that from the start.

>> With the comment fixed, and preferably with an improved commit message:
>> Reviewed-by: Markus Armbruster <address@hidden>
>
> Thanks for the reviews and suggestions.

You're welcome!