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

[Vadim Galitsyn <vadim.galitsyn@profitbricks.com>]

Hi Markus,

Thank you for the input.

> However, your query-memory looks like it could fail.

With the latest version of a patch (
http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg03475.html)
"query-memory" can fail only in two cases:

a) if in QOM there is an object of type of TYPE_PC_DIMM which has no
property of type PC_DIMM_SIZE_PROP, or
b) PC_DIMM_SIZE_PROP is not represented as an integer.

As far as I understand both (a) and (b) can happen only in case if QOM
somehow corrupted which is not a normal case
(please correct me if I am wrong).

Please also note, this is not the last version of the patch since new
functionality was suggested
to be included (NUMA information).

If patch will be accepted, I think we would need a test case for it since
command output differs once memory
was hot-added/removed per given NUMA node. When final functionality will be
negotiated, I will come up
with test scenario and test case itself.

Thank you,
Vadim


On Tue, Jun 20, 2017 at 4:10 PM, Markus Armbruster <armbru@redhat.com>
wrote:

> "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 :)
>