Re: [Qemu-devel] [PATCH v2] hmp, qmp: introduce "info memory" and "query-memory" commands

[Markus Armbruster <armbru@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Vadim Galitsyn (vadim.galitsyn@profitbricks.com) wrote:
>> Hi Dave,
>> 
>> Thank you for the feedback!
>> 
>> > I think you need to use the PRIu64 macros rather than 'lu' for the types
>> > of the ints there to keep it portable.
>> 
>> Agree, patch v3 will include this change.
>
> Thanks.
>
>> > Other than that; please add a test entry to tests/test-hmp.c
>> > and I'm guessing you'll also need a qmp test for it.
>> 
>> As far as I can see in tests/test-hmp.c, it's automatically there.
>> The routine test_info_commands() enumerates all the available "info"
>> sub-commands with "info help" and tries to execute them, so it looks
>> like no extra stuff needs to be done here (please correct me if I am wrong).
>
> Ah yes you're right; I forgot the 'info' commands were automatic.
>
>> Regarding to QMP test, I cannot find any test under tests/ which
>> does similar job as in tests/test-hmp.c. There is neither HMP commands
>> iteration nor command-specific separate tests. Under tests/qapi-schema/
>> there are set of .json's though, however, again, it looks more like general
>> tests set (not commands-specific one).
>> 
>> It seems that all the HMP related tests do general checks -- targeting
>> HMP "engine" itself. I would say, Avocado (avocado-vt) test suite can
>> be extended with "query-memory" test, and I can certainly provide one.
>> But this topic is for another mainling list.
>
> Yes hmm not sure where to put the qmp test, I just know that Markus does
> like them (I think he's out this week).

The best time to start requiring tests for new QMP commands is when the
first command is added.  We missed that opportunity.  The second best
time is right now[*].

The QMP equivalent to test-hmp.c's test_info_commands() would be good
enough for query-FOOs that can't fail.  We should have that.  It's not
fair to ask you to write it.  Not least because doing it right involves
introspection, which is going to be a bit hairy.  I guess I'll have to
do it myself[**].

However, your query-memory looks like it could fail.  Failure modes need
to be covered by test cases.  Please either explain why it can't
actually fail, or explain why testing the failure isn't practical, or
write a suitable test.  A mere sketch hacked into qmp-test.c is
perfectly okay, we can take it from there together.



[*] Or rather last March:
Message-ID: <871sugkrw5.fsf@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00296.html

[**] Surprise me!  Patches welcome :)