From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lucas Meneghel Rodrigues Subject: Re: [Autotest] [PATCH 1/2] KVM Test: Update cmd() help function in kvm_monitor.py to support parameters. Date: Mon, 12 Jul 2010 16:57:55 -0300 Message-ID: <1278964675.2584.16.camel@freedom> References: <842846872.971011277718236758.JavaMail.root@zmail04.collab.prod.int.phx2.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Michael Goldish , autotest@test.kernel.org, kvm@vger.kernel.org To: Feng Yang Return-path: Received: from mx1.redhat.com ([209.132.183.28]:52977 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752993Ab0GLT6A (ORCPT ); Mon, 12 Jul 2010 15:58:00 -0400 In-Reply-To: <842846872.971011277718236758.JavaMail.root@zmail04.collab.prod.int.phx2.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, 2010-06-28 at 05:43 -0400, Feng Yang wrote: > ----- "Michael Goldish" 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?