On Mon, 2010-06-28 at 05:43 -0400, Feng Yang wrote:
> ----- "Michael Goldish" <mgoldish@xxxxxxxxxx> wrote:
> > Why not add a wrapper for the command you're interested in?
> > If we do it your way, a test that uses cmd() with parameters will
> > have
> > to handle the human case and the QMP case separately. For example, if
> > a
> > human monitor is used the test will have to use
> >
> > vm.monitor.cmd("screendump scr.ppm")
> >
> > and if QMP is used the test will have to use
> >
> > vm.monitor.cmd("screendump filename=scr.ppm").
> >
> > but if we use a wrapper,
> >
> > vm.monitor.screendump("scr.ppm")
> >
> > will work in both cases.
> Thanks for your command.
>
> But in your way, we will have to add wrapper function for every command. I think a general function to run all qmp command during test script design is must. Or It is difficult to design a common process to test all qmp command.
>
> I still think make cmd command support parameters is better. Even we have to handle the human case and the QMP case separately.
I've thought about what would be the best approach in this situation,
and here are my findings:
* Wrappers are nice, but indeed a systematic test of all monitor
commands would be made unnecessarily complicated, since not all
functions have enough usage to justify a separate wrapper for them.
* On the other hand, it also sucks having to handle qmp and human
monitor separately.
It seems a reasonable alternative to implement the cmd() method in a way
that it picks **kwargs and then passes the appropriate params to the
underlying monitor. For example:
vm.monitor.cmd(command="screendump", filename="scr.ppm")
Would pass "screendump scr.ppm" to a human monitor, and "screendump
filename=scr.ppm" to a qmp monitor (we'd just have slightly different
implementations that handle the same **kwargs differently for each type
of monitor).
What do you guys think?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
