On Tue, Feb 26, 2013 at 10:56:39AM +0800, Yiqiao Pu wrote:
[...]
> >>
> >>>+ data[nodenr][0] = megabytes
> >>>+ elif field == 'cpus':
> >>>+ cpus = set([int(v) for v in value.split()])
> >>>+ data[nodenr][1] = cpus
> >>>+ data = [tuple(i) for i in data]
> >>>+ return data
> >>>+
> >>>+ def info_numa(self):
> >>>+ """
> >>>+ Run 'info numa' command and parse returned information
> >>>+
> >>>+ @return: An array of (ram, cpus) tuples, where ram is the RAM
> >>>size in
> >>>+ MB and cpus is a set of CPU numbers
> >>>+ """
> >>>+ r = self.human_monitor_cmd("info numa")
> >>We already have a function info() which can handle all kinds of query
> >>operation in both human monitor and qmp monitor. So how about use
> >>that directly?
> >The QMP class info() method is unpredictable and unreliable (for my use
> >case): it returns different results depending on the availability of a
> >"query_*" QMP command. It would make my test break if one day QMP
> >introduces a "query-numa" command, and I don't want that to happen.
>
> Understand your point. But if QMP support query-numa command one day,
> we also need test them in the numa test cases. So if that happen we
> need update the numa case to support it anyway.
When QEMU introduces query-numa QMP command, we can simply write code to
use it. But we must never make the test jobs break on purpose because a
new QMP command was introduced. People need to be able to upgrade QEMU
without upgrading autotest/virt-test and keep their test jobs working.
> >>>+ return self.parse_info_numa(r)
> >>>+
> >>And most of the functions in Monitor class only use for sending out
> >>the command line and getting the results without data parse. So maybe
> >>we should give this function a better name to make it differently
> >>with those just though out the results?
> >I don't understand what you mean. The method names look very clear to
> >me, and I don't see why they should have a different name just because
> >they are generic and live in the base Monitor class.
>
>
> I mean most of the functions that have the similar name with the
> monitor command now is only used for delivering the command line and
> results directly, but not analyse the results and give a certain
> format results. When we call this function we should keep this in
> mind that the info_numa is not return the results directly but will
> return "An array of (ram, cpus) tuples". It is different with other
> functions like getfd() and etc. in Monitor class. So maybe something
> like info_numa_formatted() is better for this kind of functions?
I still don't understand why the method needs a different name just
because it does some parsing. verify_status() does some verification of
the returned 'query-status' dictionary, but is not named
parse_and_verify_status(), it is named verify_status().
Actually, I would _like_ to see more methods parse the command output,
instead of expecting the callers to handle and parse the raw data. If
somebody just wants the raw data, they can simply run the low-level
cmd() method directly.
>
> >
> >(The fact that there were no existing methods independent of
> >monitor-type in the base Monitor class actually surprised me. There are
> >lots of useful methods that could be in the base class, because QMP is
> >also able to handle HMP commands if necessary.)
>
> Although QMP can handle HMP commands but in testing we should try to
> use QMP commands directly if they are exist. Otherwise the testing
> for QMP code should have less code coverage and which means we may
> miss bugs in that part of code.
When QMP commands are available, we can reimplement them in the QMP
class. But while QMP-specific methods/commands don't exist, we can get
the HMP-based methods on both classes for free if they live in the base
class and use human_monitor_cmd().
--
Eduardo
_______________________________________________
Virt-test-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/virt-test-devel
