Eric Blake <ebl...@redhat.com> writes:
> On 07/28/2017 01:32 PM, Markus Armbruster wrote:
>> Eric Blake <ebl...@redhat.com> 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 <arm...@redhat.com>
>
> Thanks for the reviews and suggestions.
You're welcome!
