Re: [PATCH 1/2] KVM Test: Update cmd() help function in kvm_monitor.py to support parameters.

Re: [PATCH 1/2] KVM Test: Update cmd() help function in kvm_monitor.py to support parameters.

[Feng Yang]

----- "Michael Goldish" <mgoldish@redhat.com> wrote:

> From: "Michael Goldish" <mgoldish@redhat.com>
> To: "Feng Yang" <fyang@redhat.com>
> Cc: autotest@test.kernel.org, kvm@vger.kernel.org
> Sent: Monday, June 28, 2010 5:18:29 PM GMT +08:00 Beijing / Chongqing / Hong Kong / Urumqi
> Subject: Re: [Autotest] [PATCH 1/2] KVM Test: Update cmd() help function in kvm_monitor.py to support parameters.
>
> On 06/28/2010 10:45 AM, Feng Yang wrote:
> > This function allow user send qmp command with parameters.
> > e.g.  balloon value=1073741824
> > Also log command to debug. 
> > 
> > Signed-off-by: Feng Yang <fyang@redhat.com>
> > ---
> >  client/tests/kvm/kvm_monitor.py |   32
> ++++++++++++++++++++++++--------
> >  1 files changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/client/tests/kvm/kvm_monitor.py
> b/client/tests/kvm/kvm_monitor.py
> > index 8440835..af9ff21 100644
> > --- a/client/tests/kvm/kvm_monitor.py
> > +++ b/client/tests/kvm/kvm_monitor.py
> > @@ -188,6 +188,7 @@ class HumanMonitor(Monitor):
> >  
> >          try:
> >              try:
> > +                logging.debug("Send command: %s" % command)
> >                  self._socket.sendall(command + "\n")
> >              except socket.error:
> >                  raise MonitorSendError("Could not send monitor
> command '%s'" %
> > @@ -258,9 +259,8 @@ class HumanMonitor(Monitor):
> >  
> >      def cmd(self, command, timeout=20):
> >          """
> > -        Send a simple command with no parameters and return its
> output.
> > -        Should only be used for commands that take no parameters
> and are
> > -        implemented under the same name for both the human and QMP
> monitors.
> > +        Send a simple command with/without parameters and return
> its output.
> > +        Implemented under the same name for both the human and QMP
> monitors.
> >  
> >          @param command: Command to send
> >          @param timeout: Time duration to wait for (qemu) prompt
> after command
> > @@ -486,6 +486,7 @@ class QMPMonitor(Monitor):
> >          try:
> >              cmdobj = self._build_cmd(cmd, args, id)
> >              try:
> > +                logging.debug("Send command: %s" % cmdobj)
> >                  self._socket.sendall(json.dumps(cmdobj) + "\n")
> >              except socket.error:
> >                  raise MonitorSendError("Could not send QMP command
> '%s'" % cmd)
> > @@ -601,11 +602,13 @@ class QMPMonitor(Monitor):
> >      # Note: all of the following functions raise exceptions in a
> similar manner
> >      # to cmd() and _get_command_output().
> >  
> > -    def cmd(self, command, timeout=20):
> > +    def cmd(self, cmdline, timeout=20):
> >          """
> > -        Send a simple command with no parameters and return its
> output.
> > -        Should only be used for commands that take no parameters
> and are
> > -        implemented under the same name for both the human and QMP
> monitors.
> > +        Send a simple command with/without parameters and return
> its output.
> > +        Implemented under the same name for both the human and QMP
> monitors.
> > +        Command with parameters should in following format e.g.:
> > +        'memsave val=0 size=10240 filename=memsave'
> > +        Command without parameter: 'memsave'
> >  
> >          @param command: Command to send
> >          @param timeout: Time duration to wait for response
> > @@ -614,7 +617,20 @@ class QMPMonitor(Monitor):
> >          @raise MonitorSendError: Raised if the command cannot be
> sent
> >          @raise MonitorProtocolError: Raised if no response is
> received
> >          """
> > -        return self._get_command_output(command, timeout=timeout)
> > +        cmdargs = cmdline.split()
> > +        command = cmdargs[0]
> > +        args = {}
> > +        for arg in cmdargs[1:]:
> > +            opt = arg.split('=')
> > +            try:
> > +                try:
> > +                    value = int(opt[1])
> > +                except ValueError:
> > +                    value = opt[1]
> > +                args[opt[0]] = value
> > +            except:
> > +                  logging.debug("Fail to create args, please check
> command")
> > +        return self._get_command_output(command, args,
> timeout=timeout)
> >  
> >  
> >      def quit(self):
> 
> 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.

Thanks again!!!
Feng Yang

> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Autotest] [PATCH 1/2] KVM Test: Update cmd() help function in kvm_monitor.py to support parameters.

[Lucas Meneghel Rodrigues]

On Mon, 2010-06-28 at 05:43 -0400, Feng Yang wrote:
> ----- "Michael Goldish" <mgoldish@redhat.com> 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?

Re: [Autotest] [PATCH 1/2] KVM Test: Update cmd() help function in kvm_monitor.py to support parameters.

[Michael Goldish]

On 07/12/2010 10:57 PM, Lucas Meneghel Rodrigues wrote:
> On Mon, 2010-06-28 at 05:43 -0400, Feng Yang wrote:
>> ----- "Michael Goldish" <mgoldish@redhat.com> 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?

Human monitor commands that take more than a single parameter will have
a problem because dicts (kwargs) are unordered, while human monitor
parameters are.

We could use a string like Feng Yang suggested both for QMP and the
human monitor.  In the human monitor case we'll just have to ignore the
keys and consider only the values in the order provided.  However, that
will only work for some of the commands.  What do we do about commands
like migrate which takes a -d flag?

The way I see it, the solution is either wrappers, or separate cmd()
syntaxes for the human monitor and QMP.  The QMP cmd() will take kwargs
(e.g. cmd(command="screendump", filename="scr.ppm")), and the human
cmd() will remain as it is today.

[PATCH 1/2] KVM Test: Update cmd() help function in kvm_monitor.py to support parameters.

[Feng Yang]

This function allow user send qmp command with parameters.
e.g.  balloon value=1073741824
Also log command to debug. 

Signed-off-by: Feng Yang <fyang@redhat.com>
---
 client/tests/kvm/kvm_monitor.py |   32 ++++++++++++++++++++++++--------
 1 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
index 8440835..af9ff21 100644
--- a/client/tests/kvm/kvm_monitor.py
+++ b/client/tests/kvm/kvm_monitor.py
@@ -188,6 +188,7 @@ class HumanMonitor(Monitor):
 
         try:
             try:
+                logging.debug("Send command: %s" % command)
                 self._socket.sendall(command + "\n")
             except socket.error:
                 raise MonitorSendError("Could not send monitor command '%s'" %
@@ -258,9 +259,8 @@ class HumanMonitor(Monitor):
 
     def cmd(self, command, timeout=20):
         """
-        Send a simple command with no parameters and return its output.
-        Should only be used for commands that take no parameters and are
-        implemented under the same name for both the human and QMP monitors.
+        Send a simple command with/without parameters and return its output.
+        Implemented under the same name for both the human and QMP monitors.
 
         @param command: Command to send
         @param timeout: Time duration to wait for (qemu) prompt after command
@@ -486,6 +486,7 @@ class QMPMonitor(Monitor):
         try:
             cmdobj = self._build_cmd(cmd, args, id)
             try:
+                logging.debug("Send command: %s" % cmdobj)
                 self._socket.sendall(json.dumps(cmdobj) + "\n")
             except socket.error:
                 raise MonitorSendError("Could not send QMP command '%s'" % cmd)
@@ -601,11 +602,13 @@ class QMPMonitor(Monitor):
     # Note: all of the following functions raise exceptions in a similar manner
     # to cmd() and _get_command_output().
 
-    def cmd(self, command, timeout=20):
+    def cmd(self, cmdline, timeout=20):
         """
-        Send a simple command with no parameters and return its output.
-        Should only be used for commands that take no parameters and are
-        implemented under the same name for both the human and QMP monitors.
+        Send a simple command with/without parameters and return its output.
+        Implemented under the same name for both the human and QMP monitors.
+        Command with parameters should in following format e.g.:
+        'memsave val=0 size=10240 filename=memsave'
+        Command without parameter: 'memsave'
 
         @param command: Command to send
         @param timeout: Time duration to wait for response
@@ -614,7 +617,20 @@ class QMPMonitor(Monitor):
         @raise MonitorSendError: Raised if the command cannot be sent
         @raise MonitorProtocolError: Raised if no response is received
         """
-        return self._get_command_output(command, timeout=timeout)
+        cmdargs = cmdline.split()
+        command = cmdargs[0]
+        args = {}
+        for arg in cmdargs[1:]:
+            opt = arg.split('=')
+            try:
+                try:
+                    value = int(opt[1])
+                except ValueError:
+                    value = opt[1]
+                args[opt[0]] = value
+            except:
+                  logging.debug("Fail to create args, please check command")
+        return self._get_command_output(command, args, timeout=timeout)
 
 
     def quit(self):
-- 
1.6.5.2

Re: [Autotest] [PATCH 1/2] KVM Test: Update cmd() help function in kvm_monitor.py to support parameters.

[Michael Goldish]

On 06/28/2010 10:45 AM, Feng Yang wrote:
> This function allow user send qmp command with parameters.
> e.g.  balloon value=1073741824
> Also log command to debug. 
> 
> Signed-off-by: Feng Yang <fyang@redhat.com>
> ---
>  client/tests/kvm/kvm_monitor.py |   32 ++++++++++++++++++++++++--------
>  1 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
> index 8440835..af9ff21 100644
> --- a/client/tests/kvm/kvm_monitor.py
> +++ b/client/tests/kvm/kvm_monitor.py
> @@ -188,6 +188,7 @@ class HumanMonitor(Monitor):
>  
>          try:
>              try:
> +                logging.debug("Send command: %s" % command)
>                  self._socket.sendall(command + "\n")
>              except socket.error:
>                  raise MonitorSendError("Could not send monitor command '%s'" %
> @@ -258,9 +259,8 @@ class HumanMonitor(Monitor):
>  
>      def cmd(self, command, timeout=20):
>          """
> -        Send a simple command with no parameters and return its output.
> -        Should only be used for commands that take no parameters and are
> -        implemented under the same name for both the human and QMP monitors.
> +        Send a simple command with/without parameters and return its output.
> +        Implemented under the same name for both the human and QMP monitors.
>  
>          @param command: Command to send
>          @param timeout: Time duration to wait for (qemu) prompt after command
> @@ -486,6 +486,7 @@ class QMPMonitor(Monitor):
>          try:
>              cmdobj = self._build_cmd(cmd, args, id)
>              try:
> +                logging.debug("Send command: %s" % cmdobj)
>                  self._socket.sendall(json.dumps(cmdobj) + "\n")
>              except socket.error:
>                  raise MonitorSendError("Could not send QMP command '%s'" % cmd)
> @@ -601,11 +602,13 @@ class QMPMonitor(Monitor):
>      # Note: all of the following functions raise exceptions in a similar manner
>      # to cmd() and _get_command_output().
>  
> -    def cmd(self, command, timeout=20):
> +    def cmd(self, cmdline, timeout=20):
>          """
> -        Send a simple command with no parameters and return its output.
> -        Should only be used for commands that take no parameters and are
> -        implemented under the same name for both the human and QMP monitors.
> +        Send a simple command with/without parameters and return its output.
> +        Implemented under the same name for both the human and QMP monitors.
> +        Command with parameters should in following format e.g.:
> +        'memsave val=0 size=10240 filename=memsave'
> +        Command without parameter: 'memsave'
>  
>          @param command: Command to send
>          @param timeout: Time duration to wait for response
> @@ -614,7 +617,20 @@ class QMPMonitor(Monitor):
>          @raise MonitorSendError: Raised if the command cannot be sent
>          @raise MonitorProtocolError: Raised if no response is received
>          """
> -        return self._get_command_output(command, timeout=timeout)
> +        cmdargs = cmdline.split()
> +        command = cmdargs[0]
> +        args = {}
> +        for arg in cmdargs[1:]:
> +            opt = arg.split('=')
> +            try:
> +                try:
> +                    value = int(opt[1])
> +                except ValueError:
> +                    value = opt[1]
> +                args[opt[0]] = value
> +            except:
> +                  logging.debug("Fail to create args, please check command")
> +        return self._get_command_output(command, args, timeout=timeout)
>  
>  
>      def quit(self):

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.