[PATCH 0/5] qmp-shell modifications for non-interactive use

[PATCH 0/5] qmp-shell modifications for non-interactive use

[Damien Hedde]

Hi,

The main idea of this series is to be a bit more user-friendly when
using qmp-shell in a non-interactive way: with an input redirection
from a file containing a list of commands.

I'm working on dynamic qapi config of a qemu machine, this would
be very useful to provide and reproduce small examples.

This series proposes the following modifications:
+ no prompt when input is non-interactive
+ an --exit-on-error option so that the shell exits on first
  error (disconnection, command parsing error, response is an error)
+ support for comment lines and escaping eol to have more reability
  in the source files.

I tested this using QMPShell. I tried HMPShell but did not findout
how to successfully use it with qemu. How do I setup an HMPShell ?.

Another "issue" I have is the handling of integers. I
deal with a lot of addresses and reading/writing them as decimal is
a bit painful (json does not support hexadecimal integer format). Do
you think of any reasonable workaround for this ? Maybe HMP shell
support this ?

Thanks for your comments,
--
Damien

Damien Hedde (5):
  python: qmp_shell: don't prompt when stdin is non-interactive
  python: qmp_shell: refactor the parsing error handling
  python: qmp_shell: refactor disconnection handling
  python: qmp_shell: add -e/--exit-on-error option
  python: qmp_shell: handle comment lines and escaped eol

 python/qemu/aqmp/qmp_shell.py | 117 ++++++++++++++++++++++++----------
 1 file changed, 83 insertions(+), 34 deletions(-)

-- 
2.35.1

[PATCH 1/5] python: qmp_shell: don't prompt when stdin is non-interactive

[Damien Hedde]

In that case, there is no echo anyway. So the prompt is just
garbage.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 python/qemu/aqmp/qmp_shell.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/python/qemu/aqmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
index d11bf54b00..a6e0f5af42 100644
--- a/python/qemu/aqmp/qmp_shell.py
+++ b/python/qemu/aqmp/qmp_shell.py
@@ -367,6 +367,8 @@ def prompt(self) -> str:
         """
         Return the current shell prompt, including a trailing space.
         """
+        if not sys.stdin.isatty():
+            return ""
         if self._transmode:
             return 'TRANS> '
         return '(QEMU) '
-- 
2.35.1

[PATCH 2/5] python: qmp_shell: refactor the parsing error handling

[Damien Hedde]

Instead of handling these error in _excute_cmd(), now
raise the exception and let read_exec_command() handle it.

Introduce QMPShellParseError (subclass of QMPShellError)
to replace QMPShellError. In next commit we will introduce
another subclass.

Introduce _print_parse_error() method because QMPShell
and HMPShell handle the printing differently.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 python/qemu/aqmp/qmp_shell.py | 51 +++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/python/qemu/aqmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
index a6e0f5af42..a1bd7d5630 100644
--- a/python/qemu/aqmp/qmp_shell.py
+++ b/python/qemu/aqmp/qmp_shell.py
@@ -136,6 +136,12 @@ class QMPShellError(QMPError):
     """
 
 
+class QMPShellParseError(QMPShellError):
+    """
+    QMP Shell Parse error class.
+    """
+
+
 class FuzzyJSON(ast.NodeTransformer):
     """
     This extension of ast.NodeTransformer filters literal "true/false/null"
@@ -246,7 +252,7 @@ def _cli_expr(self,
         for arg in tokens:
             (key, sep, val) = arg.partition('=')
             if sep != '=':
-                raise QMPShellError(
+                raise QMPShellParseError(
                     f"Expected a key=value pair, got '{arg!s}'"
                 )
 
@@ -258,14 +264,14 @@ def _cli_expr(self,
                 obj = parent.get(path, {})
                 if not isinstance(obj, dict):
                     msg = 'Cannot use "{:s}" as both leaf and non-leaf key'
-                    raise QMPShellError(msg.format('.'.join(curpath)))
+                    raise QMPShellParseError(msg.format('.'.join(curpath)))
                 parent[path] = obj
                 parent = obj
             if optpath[-1] in parent:
                 if isinstance(parent[optpath[-1]], dict):
                     msg = 'Cannot use "{:s}" as both leaf and non-leaf key'
-                    raise QMPShellError(msg.format('.'.join(curpath)))
-                raise QMPShellError(f'Cannot set "{key}" multiple times')
+                    raise QMPShellParseError(msg.format('.'.join(curpath)))
+                raise QMPShellParseError(f'Cannot set "{key}" multiple times')
             parent[optpath[-1]] = value
 
     def _build_cmd(self, cmdline: str) -> Optional[QMPMessage]:
@@ -290,7 +296,7 @@ def _build_cmd(self, cmdline: str) -> Optional[QMPMessage]:
             self._transmode = False
             if len(cmdargs) > 1:
                 msg = 'Unexpected input after close of Transaction sub-shell'
-                raise QMPShellError(msg)
+                raise QMPShellParseError(msg)
             qmpcmd = {
                 'execute': 'transaction',
                 'arguments': {'actions': self._actions}
@@ -323,17 +329,17 @@ def _print(self, qmp_message: object) -> None:
                            sort_keys=self.pretty)
         print(str(jsobj))
 
+    def _print_parse_error(self, err: QMPShellParseError) -> None:
+        print(
+            f"Error while parsing command line: {err!s}\n"
+            "command format: <command-name> "
+            "[arg-name1=arg1] ... [arg-nameN=argN",
+            file=sys.stderr
+        )
+
     def _execute_cmd(self, cmdline: str) -> bool:
-        try:
-            qmpcmd = self._build_cmd(cmdline)
-        except QMPShellError as err:
-            print(
-                f"Error while parsing command line: {err!s}\n"
-                "command format: <command-name> "
-                "[arg-name1=arg1] ... [arg-nameN=argN",
-                file=sys.stderr
-            )
-            return True
+        qmpcmd = self._build_cmd(cmdline)
+
         # For transaction mode, we may have just cached the action:
         if qmpcmd is None:
             return True
@@ -390,7 +396,11 @@ def read_exec_command(self) -> bool:
                 print(event)
             return True
 
-        return self._execute_cmd(cmdline)
+        try:
+            return self._execute_cmd(cmdline)
+        except QMPShellParseError as err:
+            self._print_parse_error(err)
+        return True
 
     def repl(self) -> Iterator[None]:
         """
@@ -456,18 +466,19 @@ def _cmd_passthrough(self, cmdline: str,
             }
         })
 
+    def _print_parse_error(self, err: QMPShellParseError) -> None:
+        print(f"{err!s}")
+
     def _execute_cmd(self, cmdline: str) -> bool:
         if cmdline.split()[0] == "cpu":
             # trap the cpu command, it requires special setting
             try:
                 idx = int(cmdline.split()[1])
                 if 'return' not in self._cmd_passthrough('info version', idx):
-                    print('bad CPU index')
-                    return True
+                    raise QMPShellParseError('bad CPU index')
                 self._cpu_index = idx
             except ValueError:
-                print('cpu command takes an integer argument')
-                return True
+                raise QMPShellParseError('cpu command takes an integer argument')
         resp = self._cmd_passthrough(cmdline, self._cpu_index)
         if resp is None:
             print('Disconnected')
-- 
2.35.1

[PATCH 3/5] python: qmp_shell: refactor disconnection handling

[Damien Hedde]

Introduce QMPShellConnectError (subclass of QMPShellError)
to handle disconnection in read_exec_command().

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 python/qemu/aqmp/qmp_shell.py | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/python/qemu/aqmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
index a1bd7d5630..cce7732ba2 100644
--- a/python/qemu/aqmp/qmp_shell.py
+++ b/python/qemu/aqmp/qmp_shell.py
@@ -142,6 +142,12 @@ class QMPShellParseError(QMPShellError):
     """
 
 
+class QMPShellConnectError(QMPShellError):
+    """
+    QMP Shell Connect error class.
+    """
+
+
 class FuzzyJSON(ast.NodeTransformer):
     """
     This extension of ast.NodeTransformer filters literal "true/false/null"
@@ -347,8 +353,7 @@ def _execute_cmd(self, cmdline: str) -> bool:
             self._print(qmpcmd)
         resp = self.cmd_obj(qmpcmd)
         if resp is None:
-            print('Disconnected')
-            return False
+            raise QMPShellConnectError('Disconnected')
         self._print(resp)
         return True
 
@@ -400,6 +405,9 @@ def read_exec_command(self) -> bool:
             return self._execute_cmd(cmdline)
         except QMPShellParseError as err:
             self._print_parse_error(err)
+        except QMPShellConnectError as err:
+            print(f"{err!s}");
+            return False
         return True
 
     def repl(self) -> Iterator[None]:
@@ -481,8 +489,7 @@ def _execute_cmd(self, cmdline: str) -> bool:
                 raise QMPShellParseError('cpu command takes an integer argument')
         resp = self._cmd_passthrough(cmdline, self._cpu_index)
         if resp is None:
-            print('Disconnected')
-            return False
+            raise QMPShellConnectError('Disconnected')
         assert 'return' in resp or 'error' in resp
         if 'return' in resp:
             # Success
-- 
2.35.1

[PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[Damien Hedde]

This option makes qmp_shell exit (with error code 1)
as soon as one of the following error occurs:
+ command parsing error
+ disconnection
+ command failure (response is an error)

_execute_cmd() method now returns None or the response
so that read_exec_command() can do the last check.

This is meant to be used in combination with an input file
redirection. It allows to store a list of commands
into a file and try to run them by qmp_shell and easily
see if it failed or not.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 python/qemu/aqmp/qmp_shell.py | 39 +++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/python/qemu/aqmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
index cce7732ba2..dd38ef8a13 100644
--- a/python/qemu/aqmp/qmp_shell.py
+++ b/python/qemu/aqmp/qmp_shell.py
@@ -11,7 +11,7 @@
 """
 Low-level QEMU shell on top of QMP.
 
-usage: qmp-shell [-h] [-H] [-N] [-v] [-p] qmp_server
+usage: qmp-shell [-h] [-H] [-N] [-v] [-p] [-e] qmp_server
 
 positional arguments:
   qmp_server            < UNIX socket path | TCP address:port >
@@ -23,6 +23,8 @@
                         Skip negotiate (for qemu-ga)
   -v, --verbose         Verbose (echo commands sent and received)
   -p, --pretty          Pretty-print JSON
+  -e, --exit-on-error   Exit when an error occurs (command parsing,
+                        disconnection and command failure)
 
 
 Start QEMU with:
@@ -177,9 +179,11 @@ class QMPShell(QEMUMonitorProtocol):
     :param address: Address of the QMP server.
     :param pretty: Pretty-print QMP messages.
     :param verbose: Echo outgoing QMP messages to console.
+    :param raise_error: Don't continue after an error occured
     """
     def __init__(self, address: SocketAddrT,
-                 pretty: bool = False, verbose: bool = False):
+                 pretty: bool = False, verbose: bool = False,
+                 raise_error: bool = False):
         super().__init__(address)
         self._greeting: Optional[QMPMessage] = None
         self._completer = QMPCompleter()
@@ -189,6 +193,7 @@ def __init__(self, address: SocketAddrT,
                                       '.qmp-shell_history')
         self.pretty = pretty
         self.verbose = verbose
+        self.raise_error = raise_error
 
     def close(self) -> None:
         # Hook into context manager of parent to save shell history.
@@ -343,19 +348,19 @@ def _print_parse_error(self, err: QMPShellParseError) -> None:
             file=sys.stderr
         )
 
-    def _execute_cmd(self, cmdline: str) -> bool:
+    def _execute_cmd(self, cmdline: str) -> Optional[QMPMessage]:
         qmpcmd = self._build_cmd(cmdline)
 
         # For transaction mode, we may have just cached the action:
         if qmpcmd is None:
-            return True
+            return None
         if self.verbose:
             self._print(qmpcmd)
         resp = self.cmd_obj(qmpcmd)
         if resp is None:
             raise QMPShellConnectError('Disconnected')
         self._print(resp)
-        return True
+        return resp
 
     def connect(self, negotiate: bool = True) -> None:
         self._greeting = super().connect(negotiate)
@@ -401,8 +406,13 @@ def read_exec_command(self) -> bool:
                 print(event)
             return True
 
+        if self.raise_error:
+            resp = self._execute_cmd(cmdline)
+            if resp and 'error' in resp:
+                raise QMPShellError(f"Command failed: {resp['error']}")
+            return True
         try:
-            return self._execute_cmd(cmdline)
+            self._execute_cmd(cmdline)
         except QMPShellParseError as err:
             self._print_parse_error(err)
         except QMPShellConnectError as err:
@@ -477,7 +487,7 @@ def _cmd_passthrough(self, cmdline: str,
     def _print_parse_error(self, err: QMPShellParseError) -> None:
         print(f"{err!s}")
 
-    def _execute_cmd(self, cmdline: str) -> bool:
+    def _execute_cmd(self, cmdline: str) -> Optional[QMPMessage]:
         if cmdline.split()[0] == "cpu":
             # trap the cpu command, it requires special setting
             try:
@@ -498,7 +508,7 @@ def _execute_cmd(self, cmdline: str) -> bool:
         else:
             # Error
             print('%s: %s' % (resp['error']['class'], resp['error']['desc']))
-        return True
+        return resp
 
     def show_banner(self, msg: str = 'Welcome to the HMP shell!') -> None:
         QMPShell.show_banner(self, msg)
@@ -523,6 +533,9 @@ def main() -> None:
                         help='Verbose (echo commands sent and received)')
     parser.add_argument('-p', '--pretty', action='store_true',
                         help='Pretty-print JSON')
+    parser.add_argument('-e', '--exit-on-error', action='store_true',
+                        help='Exit when an error occurs (command parsing,'
+                             ' disconnection and command failure)')
 
     default_server = os.environ.get('QMP_SOCKET')
     parser.add_argument('qmp_server', action='store',
@@ -541,7 +554,8 @@ def main() -> None:
         parser.error(f"Bad port number: {args.qmp_server}")
         return  # pycharm doesn't know error() is noreturn
 
-    with shell_class(address, args.pretty, args.verbose) as qemu:
+    with shell_class(address, args.pretty, args.verbose,
+                     args.exit_on_error) as qemu:
         try:
             qemu.connect(negotiate=not args.skip_negotiation)
         except ConnectError as err:
@@ -549,8 +563,11 @@ def main() -> None:
                 die(f"Couldn't connect to {args.qmp_server}: {err!s}")
             die(str(err))
 
-        for _ in qemu.repl():
-            pass
+        try:
+            for _ in qemu.repl():
+                pass
+        except QMPShellError as err:
+            die(str(err))
 
 
 if __name__ == '__main__':
-- 
2.35.1

[PATCH 5/5] python: qmp_shell: handle comment lines and escaped eol

[Damien Hedde]

In order to support more user-friendly command list file,
this commit adds the support for:
+ comment lines: line staring by '#' are ignored
+ escaped enf-of-line: line with trailing ' \' are continued
  on next one

For eol: we impose a space before the '\' in order not to trigger
the escape if the '\' is for example at the end of a value in a
'key=value' sequence.
Althought it does not have any real interest in interactive mode,
the prompt is adapted when in that case.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 python/qemu/aqmp/qmp_shell.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/python/qemu/aqmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
index dd38ef8a13..64cd31dcd6 100644
--- a/python/qemu/aqmp/qmp_shell.py
+++ b/python/qemu/aqmp/qmp_shell.py
@@ -188,6 +188,7 @@ def __init__(self, address: SocketAddrT,
         self._greeting: Optional[QMPMessage] = None
         self._completer = QMPCompleter()
         self._transmode = False
+        self._escaped_eol = False
         self._actions: List[QMPMessage] = []
         self._histfile = os.path.join(os.path.expanduser('~'),
                                       '.qmp-shell_history')
@@ -385,6 +386,8 @@ def prompt(self) -> str:
         """
         if not sys.stdin.isatty():
             return ""
+        if self._escaped_eol:
+            return '> '
         if self._transmode:
             return 'TRANS> '
         return '(QEMU) '
@@ -397,6 +400,11 @@ def read_exec_command(self) -> bool:
         """
         try:
             cmdline = input(self.prompt)
+            self._escaped_eol = True
+            while cmdline[-2:] == ' \\':
+                #only remove the trailing '\', keep the space
+                cmdline = cmdline[:-1] + input(self.prompt)
+            self._escaped_eol = False
         except EOFError:
             print()
             return False
@@ -406,6 +414,10 @@ def read_exec_command(self) -> bool:
                 print(event)
             return True
 
+        if cmdline[0] == '#':
+            #consider these lines as comments
+            return True
+
         if self.raise_error:
             resp = self._execute_cmd(cmdline)
             if resp and 'error' in resp:
-- 
2.35.1

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Markus Armbruster]

Damien Hedde <damien.hedde@greensocs.com> writes:

> Hi,
>
> The main idea of this series is to be a bit more user-friendly when
> using qmp-shell in a non-interactive way: with an input redirection
> from a file containing a list of commands.
>
> I'm working on dynamic qapi config of a qemu machine, this would
> be very useful to provide and reproduce small examples.

Why not use plain QMP for that?

[...]

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Damien Hedde]

On 2/22/22 07:10, Markus Armbruster wrote:
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
>> Hi,
>>
>> The main idea of this series is to be a bit more user-friendly when
>> using qmp-shell in a non-interactive way: with an input redirection
>> from a file containing a list of commands.
>>
>> I'm working on dynamic qapi config of a qemu machine, this would
>> be very useful to provide and reproduce small examples.
> 
> Why not use plain QMP for that?
> 
> [...]
> 
What do you mean by plain QMP ?

--
Damien

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Daniel P. Berrangé]

On Tue, Feb 22, 2022 at 08:57:03AM +0100, Damien Hedde wrote:
> 
> 
> On 2/22/22 07:10, Markus Armbruster wrote:
> > Damien Hedde <damien.hedde@greensocs.com> writes:
> > 
> > > Hi,
> > > 
> > > The main idea of this series is to be a bit more user-friendly when
> > > using qmp-shell in a non-interactive way: with an input redirection
> > > from a file containing a list of commands.
> > > 
> > > I'm working on dynamic qapi config of a qemu machine, this would
> > > be very useful to provide and reproduce small examples.
> > 
> > Why not use plain QMP for that?
> > 
> > [...]
> > 
> What do you mean by plain QMP ?

Directly connect to the socket and send the QMP JSON commands you have.

Essentially qmp-shell is designed for adhoc interactive human usage.
For automated / scripted, non-interactive usage, it is expected that
QMP is simple enough that tools just directly connect to the QMP
socket instead of using a wrapper layer.

What is the reason you want to use qmp-shell for this instead of
directly using the socket from your scripts ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Damien Hedde]

On 2/22/22 10:21, Daniel P. Berrangé wrote:
> On Tue, Feb 22, 2022 at 08:57:03AM +0100, Damien Hedde wrote:
>>
>>
>> On 2/22/22 07:10, Markus Armbruster wrote:
>>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>>
>>>> Hi,
>>>>
>>>> The main idea of this series is to be a bit more user-friendly when
>>>> using qmp-shell in a non-interactive way: with an input redirection
>>>> from a file containing a list of commands.
>>>>
>>>> I'm working on dynamic qapi config of a qemu machine, this would
>>>> be very useful to provide and reproduce small examples.
>>>
>>> Why not use plain QMP for that?
>>>
>>> [...]
>>>
>> What do you mean by plain QMP ?
> 
> Directly connect to the socket and send the QMP JSON commands you have.
> 
> Essentially qmp-shell is designed for adhoc interactive human usage.
> For automated / scripted, non-interactive usage, it is expected that
> QMP is simple enough that tools just directly connect to the QMP
> socket instead of using a wrapper layer.
> 
> What is the reason you want to use qmp-shell for this instead of
> directly using the socket from your scripts ?
> 
> Regards,
> Daniel

We have such scripts that interface with QMP directly for our own use.

Here I just wanted to propose a simple way to just send a
bunch of commands from a source file and stop if something unexpected 
happens.
Only goal is to be able to share a file on the ml and allow people to 
reproduce easily.
We can already redirect the input, but it is almost impossible to see
if something failed.

--
Damien

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Markus Armbruster]

Damien Hedde <damien.hedde@greensocs.com> writes:

> On 2/22/22 07:10, Markus Armbruster wrote:
>> Damien Hedde <damien.hedde@greensocs.com> writes:
>> 
>>> Hi,
>>>
>>> The main idea of this series is to be a bit more user-friendly when
>>> using qmp-shell in a non-interactive way: with an input redirection
>>> from a file containing a list of commands.
>>>
>>> I'm working on dynamic qapi config of a qemu machine, this would
>>> be very useful to provide and reproduce small examples.
>> Why not use plain QMP for that?
>> [...]
>> 
> What do you mean by plain QMP ?

Talk straight to QEMU without a translator:

    $ cat script
    {"execute": "qmp_capabilities"}
    {"execute": "quit"}
    $ socat -t 3 STDIO UNIX-CONNECT:$HOME/work/images/test-qmp <script
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 6}, "package": "v6.2.0-1603-gc13b8e9973"}, "capabilities": ["oob"]}}
    {"error": {"class": "CommandNotFound", "desc": "Expecting capabilities negotiation with 'qmp_capabilities'"}}
    armbru@dusky:~/work/qemu$ echo -e '{"execute":"qmp_capabilities"}{"execute":"quit"}' >script
    armbru@dusky:~/work/qemu$ echo -e '{"execute":"qmp_capabilities"}\n{"execute":"quit"}' >script
    armbru@dusky:~/work/qemu$ socat -t 3 STDIO UNIX-CONNECT:$HOME/work/images/test-qmp <script
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 6}, "package": "v6.2.0-1603-gc13b8e9973"}, "capabilities": ["oob"]}}
    {"return": {}}
    {"return": {}}
    {"timestamp": {"seconds": 1645523438, "microseconds": 951702}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}

socat also supports interactive use nicely.  Try

    $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>" UNIX-CONNECT:$HOME/path/to/socket

Helpfully blinks matching parenthesis for me.

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Daniel P. Berrangé]

On Tue, Feb 22, 2022 at 10:38:09AM +0100, Damien Hedde wrote:
> 
> 
> On 2/22/22 10:21, Daniel P. Berrangé wrote:
> > On Tue, Feb 22, 2022 at 08:57:03AM +0100, Damien Hedde wrote:
> > > 
> > > 
> > > On 2/22/22 07:10, Markus Armbruster wrote:
> > > > Damien Hedde <damien.hedde@greensocs.com> writes:
> > > > 
> > > > > Hi,
> > > > > 
> > > > > The main idea of this series is to be a bit more user-friendly when
> > > > > using qmp-shell in a non-interactive way: with an input redirection
> > > > > from a file containing a list of commands.
> > > > > 
> > > > > I'm working on dynamic qapi config of a qemu machine, this would
> > > > > be very useful to provide and reproduce small examples.
> > > > 
> > > > Why not use plain QMP for that?
> > > > 
> > > > [...]
> > > > 
> > > What do you mean by plain QMP ?
> > 
> > Directly connect to the socket and send the QMP JSON commands you have.
> > 
> > Essentially qmp-shell is designed for adhoc interactive human usage.
> > For automated / scripted, non-interactive usage, it is expected that
> > QMP is simple enough that tools just directly connect to the QMP
> > socket instead of using a wrapper layer.
> > 
> > What is the reason you want to use qmp-shell for this instead of
> > directly using the socket from your scripts ?
> > 
> > Regards,
> > Daniel
> 
> We have such scripts that interface with QMP directly for our own use.
> 
> Here I just wanted to propose a simple way to just send a
> bunch of commands from a source file and stop if something unexpected
> happens.
> Only goal is to be able to share a file on the ml and allow people to
> reproduce easily.
> We can already redirect the input, but it is almost impossible to see
> if something failed.

Yes, I see what you mean. So the problem with using 'socat' or similar
is that we fill the input with commands and response appear asynchronously,
so we can't match them up easily. This is actually a problem seen in the
block I/O tests which just send QMP stuff in a batch.

While you could do this by invoking socat once for each command, that
gets silly with the repeated QMP handshake for each command.

The thing about using qmp-shell is that it does a bunch of extra stuff
targetted at humans on top, and history tells us it isn't a good idea
to mix stuff for humans and machines in the same tool/interface.

How about instead creating a separate 'qmp-send' command that is not
much more than a "QMP-aware socat".  By which I mean, it just reads
raw QMP commands from stdin, sends each one to the server, but
crucially waits for a reply after sending each, and stops on first
error reponse.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Damien Hedde]

On 2/22/22 11:31, Daniel P. Berrangé wrote:
> On Tue, Feb 22, 2022 at 10:38:09AM +0100, Damien Hedde wrote:
>>
>>
>> Here I just wanted to propose a simple way to just send a
>> bunch of commands from a source file and stop if something unexpected
>> happens.
>> Only goal is to be able to share a file on the ml and allow people to
>> reproduce easily.
>> We can already redirect the input, but it is almost impossible to see
>> if something failed.
> 
> Yes, I see what you mean. So the problem with using 'socat' or similar
> is that we fill the input with commands and response appear asynchronously,
> so we can't match them up easily. This is actually a problem seen in the
> block I/O tests which just send QMP stuff in a batch.
> 
> While you could do this by invoking socat once for each command, that
> gets silly with the repeated QMP handshake for each command.
> 
> The thing about using qmp-shell is that it does a bunch of extra stuff
> targetted at humans on top, and history tells us it isn't a good idea
> to mix stuff for humans and machines in the same tool/interface.
> 
> How about instead creating a separate 'qmp-send' command that is not
> much more than a "QMP-aware socat".  By which I mean, it just reads
> raw QMP commands from stdin, sends each one to the server, but
> crucially waits for a reply after sending each, and stops on first
> error reponse.

By 'qmp-send' command, you mean another script in scripts/qmp ?
Yes

If we go for another script, I would rather do one with wrap
feature (like your series) to start qemu as well.

--
Damien

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Daniel P. Berrangé]

On Wed, Feb 23, 2022 at 10:57:29AM +0100, Damien Hedde wrote:
> 
> 
> On 2/22/22 11:31, Daniel P. Berrangé wrote:
> > On Tue, Feb 22, 2022 at 10:38:09AM +0100, Damien Hedde wrote:
> > > 
> > > 
> > > Here I just wanted to propose a simple way to just send a
> > > bunch of commands from a source file and stop if something unexpected
> > > happens.
> > > Only goal is to be able to share a file on the ml and allow people to
> > > reproduce easily.
> > > We can already redirect the input, but it is almost impossible to see
> > > if something failed.
> > 
> > Yes, I see what you mean. So the problem with using 'socat' or similar
> > is that we fill the input with commands and response appear asynchronously,
> > so we can't match them up easily. This is actually a problem seen in the
> > block I/O tests which just send QMP stuff in a batch.
> > 
> > While you could do this by invoking socat once for each command, that
> > gets silly with the repeated QMP handshake for each command.
> > 
> > The thing about using qmp-shell is that it does a bunch of extra stuff
> > targetted at humans on top, and history tells us it isn't a good idea
> > to mix stuff for humans and machines in the same tool/interface.
> > 
> > How about instead creating a separate 'qmp-send' command that is not
> > much more than a "QMP-aware socat".  By which I mean, it just reads
> > raw QMP commands from stdin, sends each one to the server, but
> > crucially waits for a reply after sending each, and stops on first
> > error reponse.
> 
> By 'qmp-send' command, you mean another script in scripts/qmp ?
> Yes

Yep.

> If we go for another script, I would rather do one with wrap
> feature (like your series) to start qemu as well.

Sure, that would certainly make sense.  I actually wanted to add
the wrap feature directly into the existing qmp-shell, but it was
not viable while maintaining back compat. With a new qmp-send
command you can easily make "wrap mode" supported from the start.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[John Snow]

On Wed, Feb 23, 2022 at 6:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Feb 23, 2022 at 10:57:29AM +0100, Damien Hedde wrote:
> >
> >
> > On 2/22/22 11:31, Daniel P. Berrangé wrote:
> > > On Tue, Feb 22, 2022 at 10:38:09AM +0100, Damien Hedde wrote:
> > > >
> > > >
> > > > Here I just wanted to propose a simple way to just send a
> > > > bunch of commands from a source file and stop if something unexpected
> > > > happens.
> > > > Only goal is to be able to share a file on the ml and allow people to
> > > > reproduce easily.
> > > > We can already redirect the input, but it is almost impossible to see
> > > > if something failed.
> > >
> > > Yes, I see what you mean. So the problem with using 'socat' or similar
> > > is that we fill the input with commands and response appear asynchronously,
> > > so we can't match them up easily. This is actually a problem seen in the
> > > block I/O tests which just send QMP stuff in a batch.
> > >
> > > While you could do this by invoking socat once for each command, that
> > > gets silly with the repeated QMP handshake for each command.
> > >
> > > The thing about using qmp-shell is that it does a bunch of extra stuff
> > > targetted at humans on top, and history tells us it isn't a good idea
> > > to mix stuff for humans and machines in the same tool/interface.
> > >
> > > How about instead creating a separate 'qmp-send' command that is not
> > > much more than a "QMP-aware socat".  By which I mean, it just reads
> > > raw QMP commands from stdin, sends each one to the server, but
> > > crucially waits for a reply after sending each, and stops on first
> > > error reponse.
> >
> > By 'qmp-send' command, you mean another script in scripts/qmp ?
> > Yes
>
> Yep.
>
> > If we go for another script, I would rather do one with wrap
> > feature (like your series) to start qemu as well.
>
> Sure, that would certainly make sense.  I actually wanted to add
> the wrap feature directly into the existing qmp-shell, but it was
> not viable while maintaining back compat. With a new qmp-send
> command you can easily make "wrap mode" supported from the start.
>

I'm also wary of adding scriptable interfaces to qmp-shell, but I can
see them having some value.

I'm not against the ability to add some kind of "load commands from
file" interactive command to qmp-shell, for instance. (/LOAD or /PLAY
or something?) ... but then we need to worry about what the format of
the file is and how exactly that scripting language works. It's a
design minefield.

I can also see the use in having qmp-shell autoplay a script file at
startup and then resume interactive function (Saves you some typing!),
but that opens the door to people using it as a script in and of
itself and coming to rely on it, which I really would not like to see.
A standalone qemu-send that supports launching QEMU (with args) and
taking a script file from the CLI sounds great, though.

There's still some design questions I have, though: What features do
you need out of the script file? What syntax do you intend to use?
Does it need event waits? How complex do they need to be? I'm fine
with something simple, but I can see cases where we might begin to
want slightly more power out of it. I see good utility for test
reproductions shared on the ML and in bug trackers, though.

--js

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[Daniel P. Berrangé]

On Wed, Feb 23, 2022 at 10:01:05AM -0500, John Snow wrote:
> On Wed, Feb 23, 2022 at 6:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Feb 23, 2022 at 10:57:29AM +0100, Damien Hedde wrote:
> > >
> > >
> > > On 2/22/22 11:31, Daniel P. Berrangé wrote:
> > > > On Tue, Feb 22, 2022 at 10:38:09AM +0100, Damien Hedde wrote:
> > > > >
> > > > >
> > > > > Here I just wanted to propose a simple way to just send a
> > > > > bunch of commands from a source file and stop if something unexpected
> > > > > happens.
> > > > > Only goal is to be able to share a file on the ml and allow people to
> > > > > reproduce easily.
> > > > > We can already redirect the input, but it is almost impossible to see
> > > > > if something failed.
> > > >
> > > > Yes, I see what you mean. So the problem with using 'socat' or similar
> > > > is that we fill the input with commands and response appear asynchronously,
> > > > so we can't match them up easily. This is actually a problem seen in the
> > > > block I/O tests which just send QMP stuff in a batch.
> > > >
> > > > While you could do this by invoking socat once for each command, that
> > > > gets silly with the repeated QMP handshake for each command.
> > > >
> > > > The thing about using qmp-shell is that it does a bunch of extra stuff
> > > > targetted at humans on top, and history tells us it isn't a good idea
> > > > to mix stuff for humans and machines in the same tool/interface.
> > > >
> > > > How about instead creating a separate 'qmp-send' command that is not
> > > > much more than a "QMP-aware socat".  By which I mean, it just reads
> > > > raw QMP commands from stdin, sends each one to the server, but
> > > > crucially waits for a reply after sending each, and stops on first
> > > > error reponse.
> > >
> > > By 'qmp-send' command, you mean another script in scripts/qmp ?
> > > Yes
> >
> > Yep.
> >
> > > If we go for another script, I would rather do one with wrap
> > > feature (like your series) to start qemu as well.
> >
> > Sure, that would certainly make sense.  I actually wanted to add
> > the wrap feature directly into the existing qmp-shell, but it was
> > not viable while maintaining back compat. With a new qmp-send
> > command you can easily make "wrap mode" supported from the start.
> >
> 
> I'm also wary of adding scriptable interfaces to qmp-shell, but I can
> see them having some value.
> 
> I'm not against the ability to add some kind of "load commands from
> file" interactive command to qmp-shell, for instance. (/LOAD or /PLAY
> or something?) ... but then we need to worry about what the format of
> the file is and how exactly that scripting language works. It's a
> design minefield.

My concern is that qmp-shell takes command input in a high level data
format. I don't want to see that format turn into something that
machines use, which is what is proposed initially here.

For this reason I prefer to see a separate qmp-send that solves
the automation problems, without introducing a new data format,
just directly passing raw QMP messages to/fro.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[John Snow]

On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
<damien.hedde@greensocs.com> wrote:
>
> This option makes qmp_shell exit (with error code 1)
> as soon as one of the following error occurs:
> + command parsing error
> + disconnection
> + command failure (response is an error)
>
> _execute_cmd() method now returns None or the response
> so that read_exec_command() can do the last check.
>
> This is meant to be used in combination with an input file
> redirection. It allows to store a list of commands
> into a file and try to run them by qmp_shell and easily
> see if it failed or not.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

Based on this patch, it looks like you really want something
scriptable, so I think the qemu-send idea that Dan has suggested might
be the best way to go. Are you still hoping to use the interactive
"short" QMP command format? That might be a bad idea, given how flaky
the parsing is -- and how we don't actually have a published standard
for that format. We've *never* liked the bad parsing here, so I have a
reluctance to use it in more places.

I'm having the naive idea that a script file could be as simple as a
list of QMP commands to send:

[
    {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
    ...
]

And a processing script could be something as simple as:

~~~
with open("script.json") as infile:
    script = json.load(infile)

for command in script:
    await qmp.execute_msg(command)
~~~


It's very simple to do, but the script file format is now a bit more
chatty than it used to be. You could also elide the "execute" and
"arguments" keys, perhaps:

[
    {"block-dirty-bitmap-add": {"name": ..., "node": ...},
    ...
]

And then the script only changes a little bit:

for item in script:
    for cmd, args in item.items():
        await qmp.execute(cmd, args)

but this might lose the ability to opt into "execute-oob" as one
consequence of a more terse script format.



> ---
>  python/qemu/aqmp/qmp_shell.py | 39 +++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/python/qemu/aqmp/qmp_shell.py b/python/qemu/aqmp/qmp_shell.py
> index cce7732ba2..dd38ef8a13 100644
> --- a/python/qemu/aqmp/qmp_shell.py
> +++ b/python/qemu/aqmp/qmp_shell.py
> @@ -11,7 +11,7 @@
>  """
>  Low-level QEMU shell on top of QMP.
>
> -usage: qmp-shell [-h] [-H] [-N] [-v] [-p] qmp_server
> +usage: qmp-shell [-h] [-H] [-N] [-v] [-p] [-e] qmp_server
>
>  positional arguments:
>    qmp_server            < UNIX socket path | TCP address:port >
> @@ -23,6 +23,8 @@
>                          Skip negotiate (for qemu-ga)
>    -v, --verbose         Verbose (echo commands sent and received)
>    -p, --pretty          Pretty-print JSON
> +  -e, --exit-on-error   Exit when an error occurs (command parsing,
> +                        disconnection and command failure)
>
>
>  Start QEMU with:
> @@ -177,9 +179,11 @@ class QMPShell(QEMUMonitorProtocol):
>      :param address: Address of the QMP server.
>      :param pretty: Pretty-print QMP messages.
>      :param verbose: Echo outgoing QMP messages to console.
> +    :param raise_error: Don't continue after an error occured
>      """
>      def __init__(self, address: SocketAddrT,
> -                 pretty: bool = False, verbose: bool = False):
> +                 pretty: bool = False, verbose: bool = False,
> +                 raise_error: bool = False):
>          super().__init__(address)
>          self._greeting: Optional[QMPMessage] = None
>          self._completer = QMPCompleter()
> @@ -189,6 +193,7 @@ def __init__(self, address: SocketAddrT,
>                                        '.qmp-shell_history')
>          self.pretty = pretty
>          self.verbose = verbose
> +        self.raise_error = raise_error
>
>      def close(self) -> None:
>          # Hook into context manager of parent to save shell history.
> @@ -343,19 +348,19 @@ def _print_parse_error(self, err: QMPShellParseError) -> None:
>              file=sys.stderr
>          )
>
> -    def _execute_cmd(self, cmdline: str) -> bool:
> +    def _execute_cmd(self, cmdline: str) -> Optional[QMPMessage]:
>          qmpcmd = self._build_cmd(cmdline)
>
>          # For transaction mode, we may have just cached the action:
>          if qmpcmd is None:
> -            return True
> +            return None
>          if self.verbose:
>              self._print(qmpcmd)
>          resp = self.cmd_obj(qmpcmd)
>          if resp is None:
>              raise QMPShellConnectError('Disconnected')
>          self._print(resp)
> -        return True
> +        return resp
>
>      def connect(self, negotiate: bool = True) -> None:
>          self._greeting = super().connect(negotiate)
> @@ -401,8 +406,13 @@ def read_exec_command(self) -> bool:
>                  print(event)
>              return True
>
> +        if self.raise_error:
> +            resp = self._execute_cmd(cmdline)
> +            if resp and 'error' in resp:
> +                raise QMPShellError(f"Command failed: {resp['error']}")
> +            return True
>          try:
> -            return self._execute_cmd(cmdline)
> +            self._execute_cmd(cmdline)
>          except QMPShellParseError as err:
>              self._print_parse_error(err)
>          except QMPShellConnectError as err:
> @@ -477,7 +487,7 @@ def _cmd_passthrough(self, cmdline: str,
>      def _print_parse_error(self, err: QMPShellParseError) -> None:
>          print(f"{err!s}")
>
> -    def _execute_cmd(self, cmdline: str) -> bool:
> +    def _execute_cmd(self, cmdline: str) -> Optional[QMPMessage]:
>          if cmdline.split()[0] == "cpu":
>              # trap the cpu command, it requires special setting
>              try:
> @@ -498,7 +508,7 @@ def _execute_cmd(self, cmdline: str) -> bool:
>          else:
>              # Error
>              print('%s: %s' % (resp['error']['class'], resp['error']['desc']))
> -        return True
> +        return resp
>
>      def show_banner(self, msg: str = 'Welcome to the HMP shell!') -> None:
>          QMPShell.show_banner(self, msg)
> @@ -523,6 +533,9 @@ def main() -> None:
>                          help='Verbose (echo commands sent and received)')
>      parser.add_argument('-p', '--pretty', action='store_true',
>                          help='Pretty-print JSON')
> +    parser.add_argument('-e', '--exit-on-error', action='store_true',
> +                        help='Exit when an error occurs (command parsing,'
> +                             ' disconnection and command failure)')
>
>      default_server = os.environ.get('QMP_SOCKET')
>      parser.add_argument('qmp_server', action='store',
> @@ -541,7 +554,8 @@ def main() -> None:
>          parser.error(f"Bad port number: {args.qmp_server}")
>          return  # pycharm doesn't know error() is noreturn
>
> -    with shell_class(address, args.pretty, args.verbose) as qemu:
> +    with shell_class(address, args.pretty, args.verbose,
> +                     args.exit_on_error) as qemu:
>          try:
>              qemu.connect(negotiate=not args.skip_negotiation)
>          except ConnectError as err:
> @@ -549,8 +563,11 @@ def main() -> None:
>                  die(f"Couldn't connect to {args.qmp_server}: {err!s}")
>              die(str(err))
>
> -        for _ in qemu.repl():
> -            pass
> +        try:
> +            for _ in qemu.repl():
> +                pass
> +        except QMPShellError as err:
> +            die(str(err))
>
>
>  if __name__ == '__main__':
> --
> 2.35.1
>

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[Daniel P. Berrangé]

On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
> On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
> <damien.hedde@greensocs.com> wrote:
> >
> > This option makes qmp_shell exit (with error code 1)
> > as soon as one of the following error occurs:
> > + command parsing error
> > + disconnection
> > + command failure (response is an error)
> >
> > _execute_cmd() method now returns None or the response
> > so that read_exec_command() can do the last check.
> >
> > This is meant to be used in combination with an input file
> > redirection. It allows to store a list of commands
> > into a file and try to run them by qmp_shell and easily
> > see if it failed or not.
> >
> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> 
> Based on this patch, it looks like you really want something
> scriptable, so I think the qemu-send idea that Dan has suggested might
> be the best way to go. Are you still hoping to use the interactive
> "short" QMP command format? That might be a bad idea, given how flaky
> the parsing is -- and how we don't actually have a published standard
> for that format. We've *never* liked the bad parsing here, so I have a
> reluctance to use it in more places.
> 
> I'm having the naive idea that a script file could be as simple as a
> list of QMP commands to send:
> 
> [
>     {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
>     ...
> ]

I'd really recommend against creating a new format for the script
file, especially one needing opening & closing  [] like this, as
that isn't so amenable to dynamic usage/creation. ie you can't
just append an extcra command to an existing file.

IMHO, the "file" format should be identical to the result of
capturing the socket data off the wire. ie just a concatenation
of QMP commands, with no extra wrapping / change in format.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[John Snow]

On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
> > On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
> > <damien.hedde@greensocs.com> wrote:
> > >
> > > This option makes qmp_shell exit (with error code 1)
> > > as soon as one of the following error occurs:
> > > + command parsing error
> > > + disconnection
> > > + command failure (response is an error)
> > >
> > > _execute_cmd() method now returns None or the response
> > > so that read_exec_command() can do the last check.
> > >
> > > This is meant to be used in combination with an input file
> > > redirection. It allows to store a list of commands
> > > into a file and try to run them by qmp_shell and easily
> > > see if it failed or not.
> > >
> > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> >
> > Based on this patch, it looks like you really want something
> > scriptable, so I think the qemu-send idea that Dan has suggested might
> > be the best way to go. Are you still hoping to use the interactive
> > "short" QMP command format? That might be a bad idea, given how flaky
> > the parsing is -- and how we don't actually have a published standard
> > for that format. We've *never* liked the bad parsing here, so I have a
> > reluctance to use it in more places.
> >
> > I'm having the naive idea that a script file could be as simple as a
> > list of QMP commands to send:
> >
> > [
> >     {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
> >     ...
> > ]
>
> I'd really recommend against creating a new format for the script
> file, especially one needing opening & closing  [] like this, as
> that isn't so amenable to dynamic usage/creation. ie you can't
> just append an extcra command to an existing file.
>
> IMHO, the "file" format should be identical to the result of
> capturing the socket data off the wire. ie just a concatenation
> of QMP commands, with no extra wrapping / change in format.
>

Eugh. That's just so hard to parse, because there's no off-the-shelf
tooling for "load a sequence of JSON documents". Nothing in Python
does it. :\

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[Daniel P. Berrangé]

On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote:
> On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
> > > On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
> > > <damien.hedde@greensocs.com> wrote:
> > > >
> > > > This option makes qmp_shell exit (with error code 1)
> > > > as soon as one of the following error occurs:
> > > > + command parsing error
> > > > + disconnection
> > > > + command failure (response is an error)
> > > >
> > > > _execute_cmd() method now returns None or the response
> > > > so that read_exec_command() can do the last check.
> > > >
> > > > This is meant to be used in combination with an input file
> > > > redirection. It allows to store a list of commands
> > > > into a file and try to run them by qmp_shell and easily
> > > > see if it failed or not.
> > > >
> > > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > >
> > > Based on this patch, it looks like you really want something
> > > scriptable, so I think the qemu-send idea that Dan has suggested might
> > > be the best way to go. Are you still hoping to use the interactive
> > > "short" QMP command format? That might be a bad idea, given how flaky
> > > the parsing is -- and how we don't actually have a published standard
> > > for that format. We've *never* liked the bad parsing here, so I have a
> > > reluctance to use it in more places.
> > >
> > > I'm having the naive idea that a script file could be as simple as a
> > > list of QMP commands to send:
> > >
> > > [
> > >     {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
> > >     ...
> > > ]
> >
> > I'd really recommend against creating a new format for the script
> > file, especially one needing opening & closing  [] like this, as
> > that isn't so amenable to dynamic usage/creation. ie you can't
> > just append an extcra command to an existing file.
> >
> > IMHO, the "file" format should be identical to the result of
> > capturing the socket data off the wire. ie just a concatenation
> > of QMP commands, with no extra wrapping / change in format.
> >
> 
> Eugh. That's just so hard to parse, because there's no off-the-shelf
> tooling for "load a sequence of JSON documents". Nothing in Python
> does it. :\

It isn't that hard if you require each JSON doc to be followed by
a newline.

Feed one line at a time to the JSON parser, until you get a complete
JSON doc, process that, then re-init the parser and carry on feeding
it lines until it emits the next JSON doc, and so on.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[John Snow]

On Wed, Feb 23, 2022 at 10:44 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote:
> > On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
> > > > On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
> > > > <damien.hedde@greensocs.com> wrote:
> > > > >
> > > > > This option makes qmp_shell exit (with error code 1)
> > > > > as soon as one of the following error occurs:
> > > > > + command parsing error
> > > > > + disconnection
> > > > > + command failure (response is an error)
> > > > >
> > > > > _execute_cmd() method now returns None or the response
> > > > > so that read_exec_command() can do the last check.
> > > > >
> > > > > This is meant to be used in combination with an input file
> > > > > redirection. It allows to store a list of commands
> > > > > into a file and try to run them by qmp_shell and easily
> > > > > see if it failed or not.
> > > > >
> > > > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > > >
> > > > Based on this patch, it looks like you really want something
> > > > scriptable, so I think the qemu-send idea that Dan has suggested might
> > > > be the best way to go. Are you still hoping to use the interactive
> > > > "short" QMP command format? That might be a bad idea, given how flaky
> > > > the parsing is -- and how we don't actually have a published standard
> > > > for that format. We've *never* liked the bad parsing here, so I have a
> > > > reluctance to use it in more places.
> > > >
> > > > I'm having the naive idea that a script file could be as simple as a
> > > > list of QMP commands to send:
> > > >
> > > > [
> > > >     {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
> > > >     ...
> > > > ]
> > >
> > > I'd really recommend against creating a new format for the script
> > > file, especially one needing opening & closing  [] like this, as
> > > that isn't so amenable to dynamic usage/creation. ie you can't
> > > just append an extcra command to an existing file.
> > >
> > > IMHO, the "file" format should be identical to the result of
> > > capturing the socket data off the wire. ie just a concatenation
> > > of QMP commands, with no extra wrapping / change in format.
> > >
> >
> > Eugh. That's just so hard to parse, because there's no off-the-shelf
> > tooling for "load a sequence of JSON documents". Nothing in Python
> > does it. :\
>
> It isn't that hard if you require each JSON doc to be followed by
> a newline.
>
> Feed one line at a time to the JSON parser, until you get a complete
> JSON doc, process that, then re-init the parser and carry on feeding
> it lines until it emits the next JSON doc, and so on.
>

There's two interfaces in Python:

(1) json.load(), which takes a file pointer and either returns a
single, complete JSON document or it raises an Exception. It's not
useful here at all.
(2) json.JSONDecoder().raw_decode(strbuf), which takes a string buffer
and returns a 2-tuple of a JSON Document and the position at which it
stopped decoding.

The second is what we need here, but it does require buffering the
entire file into a string first, and then iteratively calling it. It
feels like working against the grain a little bit. We also can't use
the QAPI parser, as that parser has intentionally removed support for
constructs we don't use in the qapi schema language. Boo. (Not that I
want more non-standard configuration files like that propagating,
either.)

It would be possible to generate a JSON-Schema document to describe a
script file that used a containing list construct, but impossible for
a concatenation of JSON documents. This is one of the reasons I
instinctively shy away from non-standard file formats, they tend to
cut off support for this sort of thing.

Wanting to keep the script easy to append to is legitimate. I'm keen
to hear a bit more about the use case here before I press extremely
hard in any given direction, but those are my impulses here.

--js

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[Damien Hedde]

On 2/23/22 16:44, Daniel P. Berrangé wrote:
> On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote:
>> On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>> On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
>>>> On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
>>>> <damien.hedde@greensocs.com> wrote:
>>>>>
>>>>> This option makes qmp_shell exit (with error code 1)
>>>>> as soon as one of the following error occurs:
>>>>> + command parsing error
>>>>> + disconnection
>>>>> + command failure (response is an error)
>>>>>
>>>>> _execute_cmd() method now returns None or the response
>>>>> so that read_exec_command() can do the last check.
>>>>>
>>>>> This is meant to be used in combination with an input file
>>>>> redirection. It allows to store a list of commands
>>>>> into a file and try to run them by qmp_shell and easily
>>>>> see if it failed or not.
>>>>>
>>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>>>
>>>> Based on this patch, it looks like you really want something
>>>> scriptable, so I think the qemu-send idea that Dan has suggested might
>>>> be the best way to go. Are you still hoping to use the interactive
>>>> "short" QMP command format? That might be a bad idea, given how flaky
>>>> the parsing is -- and how we don't actually have a published standard
>>>> for that format. We've *never* liked the bad parsing here, so I have a
>>>> reluctance to use it in more places.

I don't really care of the command format. I was just using the one 
expected by qmp-shell to avoid providing another script.
I think it's better not to use some standard syntax like json.
As long as we can store the commands into a file and tests them easily, 
it's ok. The crucial feature is the "stop as soon something unexpected 
happens" so that we can easily spot an issue.
>>>>
>>>> I'm having the naive idea that a script file could be as simple as a
>>>> list of QMP commands to send:
>>>>
>>>> [
>>>>      {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
>>>>      ...
>>>> ]

I used this format at some point because it's so trivial to feed into 
the QMP tools. Even used a yaml version of that to get the "human 
readability" that goes with it.

>>>
>>> I'd really recommend against creating a new format for the script
>>> file, especially one needing opening & closing  [] like this, as
>>> that isn't so amenable to dynamic usage/creation. ie you can't
>>> just append an extcra command to an existing file.
>>>
>>> IMHO, the "file" format should be identical to the result of
>>> capturing the socket data off the wire. ie just a concatenation
>>> of QMP commands, with no extra wrapping / change in format.
>>> >>
>> Eugh. That's just so hard to parse, because there's no off-the-shelf
>> tooling for "load a sequence of JSON documents". Nothing in Python
>> does it. :\
> 
> It isn't that hard if you require each JSON doc to be followed by
> a newline.
> 
> Feed one line at a time to the JSON parser, until you get a complete
> JSON doc, process that, then re-init the parser and carry on feeding
> it lines until it emits the next JSON doc, and so on.
> 

I agree it's doable. I can look into that.

It makes me think that I've managed to modify the chardev 'file' backend
a few months ago so that it can be used with an input file on the cli. 
This allowed to give such raw qmp file directly with the -qmp option 
instead of using an intermediate socket and a script issuing the same file.
But I gave up with this approach because then it can't stop if a command 
failed without hacking into the receiving side in qemu.

--
Damien

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[Damien Hedde]

On 2/23/22 17:43, Damien Hedde wrote:
> 
> 
> On 2/23/22 16:44, Daniel P. Berrangé wrote:
>> On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote:
>>> On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé 
>>> <berrange@redhat.com> wrote:
>>>>
>>>> On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
>>>>> On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
>>>>> <damien.hedde@greensocs.com> wrote:
>>>>>>
>>>>>> This option makes qmp_shell exit (with error code 1)
>>>>>> as soon as one of the following error occurs:
>>>>>> + command parsing error
>>>>>> + disconnection
>>>>>> + command failure (response is an error)
>>>>>>
>>>>>> _execute_cmd() method now returns None or the response
>>>>>> so that read_exec_command() can do the last check.
>>>>>>
>>>>>> This is meant to be used in combination with an input file
>>>>>> redirection. It allows to store a list of commands
>>>>>> into a file and try to run them by qmp_shell and easily
>>>>>> see if it failed or not.
>>>>>>
>>>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>>>>
>>>>> Based on this patch, it looks like you really want something
>>>>> scriptable, so I think the qemu-send idea that Dan has suggested might
>>>>> be the best way to go. Are you still hoping to use the interactive
>>>>> "short" QMP command format? That might be a bad idea, given how flaky
>>>>> the parsing is -- and how we don't actually have a published standard
>>>>> for that format. We've *never* liked the bad parsing here, so I have a
>>>>> reluctance to use it in more places.
> 
> I don't really care of the command format. I was just using the one 
> expected by qmp-shell to avoid providing another script.
> I think it's better not to use some standard syntax like json.
I wanted to say the opposite: it's best to use json.
> As long as we can store the commands into a file and tests them easily, 
> it's ok. The crucial feature is the "stop as soon something unexpected 
> happens" so that we can easily spot an issue.
>>>>>
>>>>> I'm having the naive idea that a script file could be as simple as a
>>>>> list of QMP commands to send:
>>>>>
>>>>> [
>>>>>      {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
>>>>>      ...
>>>>> ]
> 
> I used this format at some point because it's so trivial to feed into 
> the QMP tools. Even used a yaml version of that to get the "human 
> readability" that goes with it.
> 
>>>>
>>>> I'd really recommend against creating a new format for the script
>>>> file, especially one needing opening & closing  [] like this, as
>>>> that isn't so amenable to dynamic usage/creation. ie you can't
>>>> just append an extcra command to an existing file.
>>>>
>>>> IMHO, the "file" format should be identical to the result of
>>>> capturing the socket data off the wire. ie just a concatenation
>>>> of QMP commands, with no extra wrapping / change in format.
>>>> >>
>>> Eugh. That's just so hard to parse, because there's no off-the-shelf
>>> tooling for "load a sequence of JSON documents". Nothing in Python
>>> does it. :\
>>
>> It isn't that hard if you require each JSON doc to be followed by
>> a newline.
>>
>> Feed one line at a time to the JSON parser, until you get a complete
>> JSON doc, process that, then re-init the parser and carry on feeding
>> it lines until it emits the next JSON doc, and so on.
>>
> 
> I agree it's doable. I can look into that.
> 
> It makes me think that I've managed to modify the chardev 'file' backend
> a few months ago so that it can be used with an input file on the cli. 
> This allowed to give such raw qmp file directly with the -qmp option 
> instead of using an intermediate socket and a script issuing the same file.
> But I gave up with this approach because then it can't stop if a command 
> failed without hacking into the receiving side in qemu.
> 
> -- 
> Damien
> 
> 
> 

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[Damien Hedde]

On 2/23/22 17:18, John Snow wrote:
> On Wed, Feb 23, 2022 at 10:44 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote:
>>> On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>
>>>> On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
>>>>> On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
>>>>> <damien.hedde@greensocs.com> wrote:
>>>>>>
>>>>>> This option makes qmp_shell exit (with error code 1)
>>>>>> as soon as one of the following error occurs:
>>>>>> + command parsing error
>>>>>> + disconnection
>>>>>> + command failure (response is an error)
>>>>>>
>>>>>> _execute_cmd() method now returns None or the response
>>>>>> so that read_exec_command() can do the last check.
>>>>>>
>>>>>> This is meant to be used in combination with an input file
>>>>>> redirection. It allows to store a list of commands
>>>>>> into a file and try to run them by qmp_shell and easily
>>>>>> see if it failed or not.
>>>>>>
>>>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>>>>
>>>>> Based on this patch, it looks like you really want something
>>>>> scriptable, so I think the qemu-send idea that Dan has suggested might
>>>>> be the best way to go. Are you still hoping to use the interactive
>>>>> "short" QMP command format? That might be a bad idea, given how flaky
>>>>> the parsing is -- and how we don't actually have a published standard
>>>>> for that format. We've *never* liked the bad parsing here, so I have a
>>>>> reluctance to use it in more places.
>>>>>
>>>>> I'm having the naive idea that a script file could be as simple as a
>>>>> list of QMP commands to send:
>>>>>
>>>>> [
>>>>>      {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
>>>>>      ...
>>>>> ]
>>>>
>>>> I'd really recommend against creating a new format for the script
>>>> file, especially one needing opening & closing  [] like this, as
>>>> that isn't so amenable to dynamic usage/creation. ie you can't
>>>> just append an extcra command to an existing file.
>>>>
>>>> IMHO, the "file" format should be identical to the result of
>>>> capturing the socket data off the wire. ie just a concatenation
>>>> of QMP commands, with no extra wrapping / change in format.
>>>>
>>>
>>> Eugh. That's just so hard to parse, because there's no off-the-shelf
>>> tooling for "load a sequence of JSON documents". Nothing in Python
>>> does it. :\
>>
>> It isn't that hard if you require each JSON doc to be followed by
>> a newline.
>>
>> Feed one line at a time to the JSON parser, until you get a complete
>> JSON doc, process that, then re-init the parser and carry on feeding
>> it lines until it emits the next JSON doc, and so on.
>>
> 
> There's two interfaces in Python:
> 
> (1) json.load(), which takes a file pointer and either returns a
> single, complete JSON document or it raises an Exception. It's not
> useful here at all.
> (2) json.JSONDecoder().raw_decode(strbuf), which takes a string buffer
> and returns a 2-tuple of a JSON Document and the position at which it
> stopped decoding.
> 
> The second is what we need here, but it does require buffering the
> entire file into a string first, and then iteratively calling it. It
> feels like working against the grain a little bit. We also can't use
> the QAPI parser, as that parser has intentionally removed support for
> constructs we don't use in the qapi schema language. Boo. (Not that I
> want more non-standard configuration files like that propagating,
> either.)
> 
> It would be possible to generate a JSON-Schema document to describe a
> script file that used a containing list construct, but impossible for
> a concatenation of JSON documents. This is one of the reasons I
> instinctively shy away from non-standard file formats, they tend to
> cut off support for this sort of thing.
> 
> Wanting to keep the script easy to append to is legitimate. I'm keen
> to hear a bit more about the use case here before I press extremely
> hard in any given direction, but those are my impulses here.
> 

The use case is to be able to feed qemu with a bunch of commands we 
expect to succeed and let qemu continue (unlike Daniel's wrap use case, 
we don't want to quit qemu after the last command).

Typically it's the use case I present in the following cover-letter:
https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.hedde@greensocs.com/

--
Damien

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[Daniel P. Berrangé]

On Wed, Feb 23, 2022 at 11:18:26AM -0500, John Snow wrote:
> On Wed, Feb 23, 2022 at 10:44 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote:
> > > On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
> > > > > On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
> > > > > <damien.hedde@greensocs.com> wrote:
> > > > > >
> > > > > > This option makes qmp_shell exit (with error code 1)
> > > > > > as soon as one of the following error occurs:
> > > > > > + command parsing error
> > > > > > + disconnection
> > > > > > + command failure (response is an error)
> > > > > >
> > > > > > _execute_cmd() method now returns None or the response
> > > > > > so that read_exec_command() can do the last check.
> > > > > >
> > > > > > This is meant to be used in combination with an input file
> > > > > > redirection. It allows to store a list of commands
> > > > > > into a file and try to run them by qmp_shell and easily
> > > > > > see if it failed or not.
> > > > > >
> > > > > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > > > >
> > > > > Based on this patch, it looks like you really want something
> > > > > scriptable, so I think the qemu-send idea that Dan has suggested might
> > > > > be the best way to go. Are you still hoping to use the interactive
> > > > > "short" QMP command format? That might be a bad idea, given how flaky
> > > > > the parsing is -- and how we don't actually have a published standard
> > > > > for that format. We've *never* liked the bad parsing here, so I have a
> > > > > reluctance to use it in more places.
> > > > >
> > > > > I'm having the naive idea that a script file could be as simple as a
> > > > > list of QMP commands to send:
> > > > >
> > > > > [
> > > > >     {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
> > > > >     ...
> > > > > ]
> > > >
> > > > I'd really recommend against creating a new format for the script
> > > > file, especially one needing opening & closing  [] like this, as
> > > > that isn't so amenable to dynamic usage/creation. ie you can't
> > > > just append an extcra command to an existing file.
> > > >
> > > > IMHO, the "file" format should be identical to the result of
> > > > capturing the socket data off the wire. ie just a concatenation
> > > > of QMP commands, with no extra wrapping / change in format.
> > > >
> > >
> > > Eugh. That's just so hard to parse, because there's no off-the-shelf
> > > tooling for "load a sequence of JSON documents". Nothing in Python
> > > does it. :\
> >
> > It isn't that hard if you require each JSON doc to be followed by
> > a newline.
> >
> > Feed one line at a time to the JSON parser, until you get a complete
> > JSON doc, process that, then re-init the parser and carry on feeding
> > it lines until it emits the next JSON doc, and so on.
> >
> 
> There's two interfaces in Python:
> 
> (1) json.load(), which takes a file pointer and either returns a
> single, complete JSON document or it raises an Exception. It's not
> useful here at all.
> (2) json.JSONDecoder().raw_decode(strbuf), which takes a string buffer
> and returns a 2-tuple of a JSON Document and the position at which it
> stopped decoding.

Yes, the latter would do it, but you can also be lazy and just
repeatedly call json.loads() until you get a valid parse

$ cat demo.py
import json

cmds = []
bits = []
with open("qmp.txt", "r") as fh:
    for line in fh:
        bits.append(line)
        try:
            cmdstr = "".join(bits)
            cmds.append(json.loads(cmdstr))
            bits = []
        except json.JSONDecodeError:
            pass


for cmd in cmds:
    print("Command: %s" % cmd)


$ cat qmp.txt
{ "execute": "qmp_capabilities" }
{ "execute": "blockdev-add",
    "arguments": {
        "node-name": "drive0",
        "driver": "file",
        "filename": "$TEST_IMG"
    }
}
{ "execute": "blockdev-add",
    "arguments": {
        "driver": "$IMGFMT",
        "node-name": "drive0-debug",
        "file": {
            "driver": "blkdebug",
            "image": "drive0",
            "inject-error": [{
                "event": "l2_load"
            }]
        }
    }
}
{ "execute": "human-monitor-command",
    "arguments": {
        "command-line": "qemu-io drive0-debug \"read 0 512\""
    }
}
{ "execute": "quit" }


$ python demo.py
Command: {'execute': 'qmp_capabilities'}
Command: {'execute': 'blockdev-add', 'arguments': {'node-name': 'drive0', 'driver': 'file', 'filename': '$TEST_IMG'}}
Command: {'execute': 'blockdev-add', 'arguments': {'driver': '$IMGFMT', 'node-name': 'drive0-debug', 'file': {'driver': 'blkdebug', 'image': 'drive0', 'inject-error': [{'event': 'l2_load'}]}}}
Command: {'execute': 'human-monitor-command', 'arguments': {'command-line': 'qemu-io drive0-debug "read 0 512"'}}
Command: {'execute': 'quit'}


> Wanting to keep the script easy to append to is legitimate. I'm keen
> to hear a bit more about the use case here before I press extremely
> hard in any given direction, but those are my impulses here.

We can see examples of where this could be used in the I/O tests

eg in tests/qemu-iotests/071, a frequent pattern is:

run_qemu <<EOF
{ "execute": "qmp_capabilities" }
{ "execute": "blockdev-add",
    "arguments": {
        "node-name": "drive0",
        "driver": "file",
        "filename": "$TEST_IMG"
    }
}
{ "execute": "blockdev-add",
    "arguments": {
        "driver": "$IMGFMT",
        "node-name": "drive0-debug",
        "file": {
            "driver": "blkdebug",
            "image": "drive0",
            "inject-error": [{
                "event": "l2_load"
            }]
        }
    }
}
{ "execute": "human-monitor-command",
    "arguments": {
        "command-line": 'qemu-io drive0-debug "read 0 512"'
    }
}
{ "execute": "quit" }
EOF


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[John Snow]

On Wed, Feb 23, 2022 at 12:09 PM Damien Hedde
<damien.hedde@greensocs.com> wrote:
>
>
>
> On 2/23/22 17:18, John Snow wrote:
> > On Wed, Feb 23, 2022 at 10:44 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >>
> >> On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote:
> >>> On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >>>>
> >>>> On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
> >>>>> On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
> >>>>> <damien.hedde@greensocs.com> wrote:
> >>>>>>
> >>>>>> This option makes qmp_shell exit (with error code 1)
> >>>>>> as soon as one of the following error occurs:
> >>>>>> + command parsing error
> >>>>>> + disconnection
> >>>>>> + command failure (response is an error)
> >>>>>>
> >>>>>> _execute_cmd() method now returns None or the response
> >>>>>> so that read_exec_command() can do the last check.
> >>>>>>
> >>>>>> This is meant to be used in combination with an input file
> >>>>>> redirection. It allows to store a list of commands
> >>>>>> into a file and try to run them by qmp_shell and easily
> >>>>>> see if it failed or not.
> >>>>>>
> >>>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> >>>>>
> >>>>> Based on this patch, it looks like you really want something
> >>>>> scriptable, so I think the qemu-send idea that Dan has suggested might
> >>>>> be the best way to go. Are you still hoping to use the interactive
> >>>>> "short" QMP command format? That might be a bad idea, given how flaky
> >>>>> the parsing is -- and how we don't actually have a published standard
> >>>>> for that format. We've *never* liked the bad parsing here, so I have a
> >>>>> reluctance to use it in more places.
> >>>>>
> >>>>> I'm having the naive idea that a script file could be as simple as a
> >>>>> list of QMP commands to send:
> >>>>>
> >>>>> [
> >>>>>      {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
> >>>>>      ...
> >>>>> ]
> >>>>
> >>>> I'd really recommend against creating a new format for the script
> >>>> file, especially one needing opening & closing  [] like this, as
> >>>> that isn't so amenable to dynamic usage/creation. ie you can't
> >>>> just append an extcra command to an existing file.
> >>>>
> >>>> IMHO, the "file" format should be identical to the result of
> >>>> capturing the socket data off the wire. ie just a concatenation
> >>>> of QMP commands, with no extra wrapping / change in format.
> >>>>
> >>>
> >>> Eugh. That's just so hard to parse, because there's no off-the-shelf
> >>> tooling for "load a sequence of JSON documents". Nothing in Python
> >>> does it. :\
> >>
> >> It isn't that hard if you require each JSON doc to be followed by
> >> a newline.
> >>
> >> Feed one line at a time to the JSON parser, until you get a complete
> >> JSON doc, process that, then re-init the parser and carry on feeding
> >> it lines until it emits the next JSON doc, and so on.
> >>
> >
> > There's two interfaces in Python:
> >
> > (1) json.load(), which takes a file pointer and either returns a
> > single, complete JSON document or it raises an Exception. It's not
> > useful here at all.
> > (2) json.JSONDecoder().raw_decode(strbuf), which takes a string buffer
> > and returns a 2-tuple of a JSON Document and the position at which it
> > stopped decoding.
> >
> > The second is what we need here, but it does require buffering the
> > entire file into a string first, and then iteratively calling it. It
> > feels like working against the grain a little bit. We also can't use
> > the QAPI parser, as that parser has intentionally removed support for
> > constructs we don't use in the qapi schema language. Boo. (Not that I
> > want more non-standard configuration files like that propagating,
> > either.)
> >
> > It would be possible to generate a JSON-Schema document to describe a
> > script file that used a containing list construct, but impossible for
> > a concatenation of JSON documents. This is one of the reasons I
> > instinctively shy away from non-standard file formats, they tend to
> > cut off support for this sort of thing.
> >
> > Wanting to keep the script easy to append to is legitimate. I'm keen
> > to hear a bit more about the use case here before I press extremely
> > hard in any given direction, but those are my impulses here.
> >
>
> The use case is to be able to feed qemu with a bunch of commands we
> expect to succeed and let qemu continue (unlike Daniel's wrap use case,
> we don't want to quit qemu after the last command).
>
> Typically it's the use case I present in the following cover-letter:
> https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.hedde@greensocs.com/
>

OK (Sorry for blowing this out into a bigger ordeal than you had maybe
hoped for. I want to get you happy and on your way ASAP, I promise)

So, I see comments and simple QMP commands using the short-hand
format. If I understand correctly, you want this script upstream so
that you don't have to re-engineer the hack every time I shift
something around in qmp-shell, and so that examples can be easily
shared and reproduced between parties. Good reasons, so I want to help
you out and get something merged. (An aside: In the past I have just
copy-pasted stuff into my qmp-shell terminal. It's less reproducible
for people who aren't used to using the tool, but it's been just
enough juice for me in the past. I empathize with wanting to give
someone a single-shot command they can copy-paste and have it Just
Work.)

Some observations:

(1) Comments we don't have in JSON, but we could use YAML instead, I'm
fine with that personally. It does mean a new format (We don't have
QMP-as-YAML anywhere in-tree), but it's one that maps so directly to
JSON that I don't really consider it a problem. We might need to add a
YAML dependency to the Python tools, but we can make the feature
optional based on the presence of the yaml parser package. We can make
it print a nice error explaining why it's not supported when pyyaml
isn't found. It's an extremely common library. However, using YAML
*requires* you to use a parsing tool (qmp-shell, qmp-send, whichever)
to translate it to the wire format. Slight downside, but then again:
It seems likely that we'll have different design priorities that
separate a human-readable script file from the actual wire protocol
format.

We could also use JSON5, although that doesn't have native support in
Python, so I am somewhat against it for that reason.
We could also use the same "custom" format we use in qapi-gen for the
QAPI schema, since at least we already use it in the tree. I'm not a
big fan, but hey, there's precedent. (The custom QAPI parser would
need to be expanded to allow the full spectrum of JSON value types and
split out from the QAPI generator. It's possible to do, and I've
thought about doing it before.)

Using Dan's suggestion and storing commands as a sequence of JSON
documents works (And avoids irritating anyone over a new format), but
doesn't allow us the chance to use comments. That's slightly less
useful for sharing little examples that are also human-readable on the
ML. A plus side is that it's easy to just copy-paste the commands and
toss them into socat if you're used to testing QMP that way, which a
lot of QEMU devs seem to be. A downside is that anything that exceeds
the complexity of just "pasting QMP straight into the socket" is not
possible with this format, see point #3 below.

(2) The short-hand format qmp-shell uses is problematic because it's
non-standard and doesn't handle nested data very well. block device
commands in particular are a bit fragile -- again because we don't
have a parser in Python that is capable of starting from '{' and
reading until the closing '}', so we require that embedded JSON
arguments have no spaces at all. It's not the best, but we've never
come up with anything better. A saving grace has been that at least
this syntax was limited to the interactive interface. This is probably
the main reason people wince when extending this format to a script
we'll need to maintain backwards compatibility for.

(3) The main benefit to using a script file like this is to be able to
stop at the first error. Valid. Do you anticipate needing more
advanced "waiting" constructs, though? For instance, calling a block
formatting command and waiting until the formatting is complete before
continuing; calling any block job, etc. I am wondering if we need to
consider event-waits as part of this design space or not. Obviously
including them will complicate things a bit, but I might want to leave
open the option for future inclusion if we want to expand in that
direction.

Basically, we just don't have a good syntax for "human readable QMP"
and we've never agreed on one. The one we have in qmp-shell is fairly
widely disliked, but at least confined to human usage.

So, ways out of the swamp:

(A) I'm completely fine with adding an interactive command to
qmp-shell ("/play file.txt") or even --play-script file.txt to the
command line so long as the qmp-shell doesn't actually exit
automatically. I.e., it stays interactive. Then I don't really care
what the file format is, because it's not a scripting interface. It's
just a neat little feature for convenience.

(B) qemu-send is a good idea, but we need to decide on the storage
format. Dan's idea involves the absolute least amount of design work.
You could possibly add comments by pre-filtering the input and then
passing it off to json.loads(), but it does preclude any more advanced
constructs like timed waits, event waits, etc. Even so, this approach
is *really* easy for me to accept into the tree.

(C) qemu-send, but with a "custom" script format. YAML, JSON5, the
QAPI JSON dialect. Opens Pandora's Box, but has merit.


As an FYI, I am likely to embark on option (C) myself for separate
reasons for the aqmp-tui tool that I am still working on. I want to
add some advanced features to that tool:
- Save command history as a playback file
- Load playback files and re-play them at the server
- Generate an iotest stub based on the current history / a playback file

That's a bigger design problem and I have no intention of burdening
you with it, but it does make me wonder if we choose the simplest
option right now that I'll have another problem dealing with obsoleted
script files in the future when I want to deprecate qmp-shell. Eh, I
can always augment a theoretical qmp-send tool to "upgrade" in the
future to be able to read either "v1" or "v2" formats based on what it
sees. Making that kind of behavior very easy to perform in the future
somehow would be greatly appreciated.

*exhale*

Alright, so which way should we head?

--js

Re: [PATCH 4/5] python: qmp_shell: add -e/--exit-on-error option

[Damien Hedde]

On 2/23/22 19:20, John Snow wrote:
> On Wed, Feb 23, 2022 at 12:09 PM Damien Hedde
> <damien.hedde@greensocs.com> wrote:
>>
>>
>>
>> On 2/23/22 17:18, John Snow wrote:
>>> On Wed, Feb 23, 2022 at 10:44 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>
>>>> On Wed, Feb 23, 2022 at 10:41:11AM -0500, John Snow wrote:
>>>>> On Wed, Feb 23, 2022 at 10:27 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>>>
>>>>>> On Wed, Feb 23, 2022 at 10:22:11AM -0500, John Snow wrote:
>>>>>>> On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
>>>>>>> <damien.hedde@greensocs.com> wrote:
>>>>>>>>
>>>>>>>> This option makes qmp_shell exit (with error code 1)
>>>>>>>> as soon as one of the following error occurs:
>>>>>>>> + command parsing error
>>>>>>>> + disconnection
>>>>>>>> + command failure (response is an error)
>>>>>>>>
>>>>>>>> _execute_cmd() method now returns None or the response
>>>>>>>> so that read_exec_command() can do the last check.
>>>>>>>>
>>>>>>>> This is meant to be used in combination with an input file
>>>>>>>> redirection. It allows to store a list of commands
>>>>>>>> into a file and try to run them by qmp_shell and easily
>>>>>>>> see if it failed or not.
>>>>>>>>
>>>>>>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>>>>>>
>>>>>>> Based on this patch, it looks like you really want something
>>>>>>> scriptable, so I think the qemu-send idea that Dan has suggested might
>>>>>>> be the best way to go. Are you still hoping to use the interactive
>>>>>>> "short" QMP command format? That might be a bad idea, given how flaky
>>>>>>> the parsing is -- and how we don't actually have a published standard
>>>>>>> for that format. We've *never* liked the bad parsing here, so I have a
>>>>>>> reluctance to use it in more places.
>>>>>>>
>>>>>>> I'm having the naive idea that a script file could be as simple as a
>>>>>>> list of QMP commands to send:
>>>>>>>
>>>>>>> [
>>>>>>>       {"execute": "block-dirty-bitmap-add", "arguments": { ... }},
>>>>>>>       ...
>>>>>>> ]
>>>>>>
>>>>>> I'd really recommend against creating a new format for the script
>>>>>> file, especially one needing opening & closing  [] like this, as
>>>>>> that isn't so amenable to dynamic usage/creation. ie you can't
>>>>>> just append an extcra command to an existing file.
>>>>>>
>>>>>> IMHO, the "file" format should be identical to the result of
>>>>>> capturing the socket data off the wire. ie just a concatenation
>>>>>> of QMP commands, with no extra wrapping / change in format.
>>>>>>
>>>>>
>>>>> Eugh. That's just so hard to parse, because there's no off-the-shelf
>>>>> tooling for "load a sequence of JSON documents". Nothing in Python
>>>>> does it. :\
>>>>
>>>> It isn't that hard if you require each JSON doc to be followed by
>>>> a newline.
>>>>
>>>> Feed one line at a time to the JSON parser, until you get a complete
>>>> JSON doc, process that, then re-init the parser and carry on feeding
>>>> it lines until it emits the next JSON doc, and so on.
>>>>
>>>
>>> There's two interfaces in Python:
>>>
>>> (1) json.load(), which takes a file pointer and either returns a
>>> single, complete JSON document or it raises an Exception. It's not
>>> useful here at all.
>>> (2) json.JSONDecoder().raw_decode(strbuf), which takes a string buffer
>>> and returns a 2-tuple of a JSON Document and the position at which it
>>> stopped decoding.
>>>
>>> The second is what we need here, but it does require buffering the
>>> entire file into a string first, and then iteratively calling it. It
>>> feels like working against the grain a little bit. We also can't use
>>> the QAPI parser, as that parser has intentionally removed support for
>>> constructs we don't use in the qapi schema language. Boo. (Not that I
>>> want more non-standard configuration files like that propagating,
>>> either.)
>>>
>>> It would be possible to generate a JSON-Schema document to describe a
>>> script file that used a containing list construct, but impossible for
>>> a concatenation of JSON documents. This is one of the reasons I
>>> instinctively shy away from non-standard file formats, they tend to
>>> cut off support for this sort of thing.
>>>
>>> Wanting to keep the script easy to append to is legitimate. I'm keen
>>> to hear a bit more about the use case here before I press extremely
>>> hard in any given direction, but those are my impulses here.
>>>
>>
>> The use case is to be able to feed qemu with a bunch of commands we
>> expect to succeed and let qemu continue (unlike Daniel's wrap use case,
>> we don't want to quit qemu after the last command).
>>
>> Typically it's the use case I present in the following cover-letter:
>> https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.hedde@greensocs.com/
>>
> 
> OK (Sorry for blowing this out into a bigger ordeal than you had maybe
> hoped for. I want to get you happy and on your way ASAP, I promise)
> 
> So, I see comments and simple QMP commands using the short-hand
> format. If I understand correctly, you want this script upstream so
> that you don't have to re-engineer the hack every time I shift
> something around in qmp-shell, and so that examples can be easily
> shared and reproduced between parties. Good reasons, so I want to help
> you out and get something merged. (An aside: In the past I have just
> copy-pasted stuff into my qmp-shell terminal. It's less reproducible
> for people who aren't used to using the tool, but it's been just
> enough juice for me in the past. I empathize with wanting to give
> someone a single-shot command they can copy-paste and have it Just
> Work.)
> 
> Some observations:
> 
> (1) Comments we don't have in JSON, but we could use YAML instead, I'm
> fine with that personally. It does mean a new format (We don't have
> QMP-as-YAML anywhere in-tree), but it's one that maps so directly to
> JSON that I don't really consider it a problem. We might need to add a
> YAML dependency to the Python tools, but we can make the feature
> optional based on the presence of the yaml parser package. We can make
> it print a nice error explaining why it's not supported when pyyaml
> isn't found. It's an extremely common library. However, using YAML
> *requires* you to use a parsing tool (qmp-shell, qmp-send, whichever)
> to translate it to the wire format. Slight downside, but then again:
> It seems likely that we'll have different design priorities that
> separate a human-readable script file from the actual wire protocol
> format.
> 
> We could also use JSON5, although that doesn't have native support in
> Python, so I am somewhat against it for that reason.
> We could also use the same "custom" format we use in qapi-gen for the
> QAPI schema, since at least we already use it in the tree. I'm not a
> big fan, but hey, there's precedent. (The custom QAPI parser would
> need to be expanded to allow the full spectrum of JSON value types and
> split out from the QAPI generator. It's possible to do, and I've
> thought about doing it before.)
> 
> Using Dan's suggestion and storing commands as a sequence of JSON
> documents works (And avoids irritating anyone over a new format), but
> doesn't allow us the chance to use comments. That's slightly less
> useful for sharing little examples that are also human-readable on the
> ML. A plus side is that it's easy to just copy-paste the commands and
> toss them into socat if you're used to testing QMP that way, which a
> lot of QEMU devs seem to be. A downside is that anything that exceeds
> the complexity of just "pasting QMP straight into the socket" is not
> possible with this format, see point #3 below.
> 
> (2) The short-hand format qmp-shell uses is problematic because it's
> non-standard and doesn't handle nested data very well. block device
> commands in particular are a bit fragile -- again because we don't
> have a parser in Python that is capable of starting from '{' and
> reading until the closing '}', so we require that embedded JSON
> arguments have no spaces at all. It's not the best, but we've never
> come up with anything better. A saving grace has been that at least
> this syntax was limited to the interactive interface. This is probably
> the main reason people wince when extending this format to a script
> we'll need to maintain backwards compatibility for.
> 
> (3) The main benefit to using a script file like this is to be able to
> stop at the first error. Valid. Do you anticipate needing more
> advanced "waiting" constructs, though? For instance, calling a block
> formatting command and waiting until the formatting is complete before
> continuing; calling any block job, etc. I am wondering if we need to
> consider event-waits as part of this design space or not. Obviously
> including them will complicate things a bit, but I might want to leave
> open the option for future inclusion if we want to expand in that
> direction.
> 
> Basically, we just don't have a good syntax for "human readable QMP"
> and we've never agreed on one. The one we have in qmp-shell is fairly
> widely disliked, but at least confined to human usage.
> 
> So, ways out of the swamp:
> 
> (A) I'm completely fine with adding an interactive command to
> qmp-shell ("/play file.txt") or even --play-script file.txt to the
> command line so long as the qmp-shell doesn't actually exit
> automatically. I.e., it stays interactive. Then I don't really care
> what the file format is, because it's not a scripting interface. It's
> just a neat little feature for convenience.
> 
> (B) qemu-send is a good idea, but we need to decide on the storage
> format. Dan's idea involves the absolute least amount of design work.
> You could possibly add comments by pre-filtering the input and then
> passing it off to json.loads(), but it does preclude any more advanced
> constructs like timed waits, event waits, etc. Even so, this approach
> is *really* easy for me to accept into the tree.
For comment we could even consider adding them under the key "comment" 
and filter them after json.loads().

> 
> (C) qemu-send, but with a "custom" script format. YAML, JSON5, the
> QAPI JSON dialect. Opens Pandora's Box, but has merit.
> 
> 
> As an FYI, I am likely to embark on option (C) myself for separate
> reasons for the aqmp-tui tool that I am still working on. I want to
> add some advanced features to that tool:
> - Save command history as a playback file
> - Load playback files and re-play them at the server
> - Generate an iotest stub based on the current history / a playback file
> 
> That's a bigger design problem and I have no intention of burdening
> you with it, but it does make me wonder if we choose the simplest
> option right now that I'll have another problem dealing with obsoleted
> script files in the future when I want to deprecate qmp-shell. Eh, I
> can always augment a theoretical qmp-send tool to "upgrade" in the
> future to be able to read either "v1" or "v2" formats based on what it
> sees. Making that kind of behavior very easy to perform in the future
> somehow would be greatly appreciated.
> 
> *exhale*
> 
> Alright, so which way should we head?
> 
> --js
> 

My initial idea was to keep it simple. And in our case, I think we have 
to handle only 'deterministic' commands: we deal almost exclusively with 
commands creating an object, a device, or setting a property.
I mean we know what the commands produce, so we don't need to parse the 
result. We just expect created objects to be there. Checking if the 
command failed is the only relevant test.

Comments/eol are the icing on the cake, there are numerous ways to get 
rid of them before giving a file to the script.

I don't think introducing a new format is worth-it for our use case. Our 
work is still at the beginning and I don't have clear-enough view of the 
future to give a list a features.

So any option would do.

Another feature I didn't think to integrate before seeing Daniel's 
patches is the wrap to avoid requiring 2 terminals.

So maybe if going for option (A) that need to go in the qmp-shell-wrap 
or maybe both ? I suppose we want to keep them aligned.

Regarding the wrap, most of my use cases imply that I can for example 
interact with qemu stdin (for guest interaction). So I suppose this 
eliminate (A) because having an interactive qmp-shell and an interactive 
qemu would be really messy.

So having a:
$qmp-something --file commands.txt --start-qemu [qemu command line]
would be my preference.

--
Damien

Re: [PATCH 0/5] qmp-shell modifications for non-interactive use

[John Snow]

On Mon, Feb 21, 2022 at 10:55 AM Damien Hedde
<damien.hedde@greensocs.com> wrote:
>
> Hi,
>
> The main idea of this series is to be a bit more user-friendly when
> using qmp-shell in a non-interactive way: with an input redirection
> from a file containing a list of commands.
>
> I'm working on dynamic qapi config of a qemu machine, this would
> be very useful to provide and reproduce small examples.
>
> This series proposes the following modifications:
> + no prompt when input is non-interactive
> + an --exit-on-error option so that the shell exits on first
>   error (disconnection, command parsing error, response is an error)
> + support for comment lines and escaping eol to have more reability
>   in the source files.
>
> I tested this using QMPShell. I tried HMPShell but did not findout
> how to successfully use it with qemu. How do I setup an HMPShell ?.
>

Should be the same. It's just passing HMP commands through the QMP
layer using {"execute": "human-monitor-command", ...}.

> ./qmp-shell -H ../../bin/git/monitor.sock
Welcome to the HMP shell!
Connected to QEMU 6.2.50

(QEMU) help
announce_self [interfaces] [id] -- Trigger GARP/RARP announcements

... (and many more lines.)

HMP is a completely different interface, with different commands, etc.
It's meant for human users, but we are trying to remove any reasons to
use it. It has its uses for some debug commands that we don't want to
support over the QMP interface, and for some commands that aren't safe
and might freeze, etc. Dan has spent a lot of time and effort
migrating the save state commands over to QMP, for example.

> Another "issue" I have is the handling of integers. I
> deal with a lot of addresses and reading/writing them as decimal is
> a bit painful (json does not support hexadecimal integer format). Do
> you think of any reasonable workaround for this ? Maybe HMP shell
> support this ?

Take a look at `def _parse_value(cls, val: str) -> object:`

int("0x13") is going to raise an Exception, so this won't be perceived
as a numerical value. You can use int(val, base=0) to have Python
guess the base from the string given (which allows for "0x", "0b" and
maybe some others.) This might cause a regression if anyone was using
this to pass "0x13" as a string on purpose.

It might also be the case that the FuzzyJSON portion of the parser
already accepts hexadecimal numbers. You'd have to check, but if you
take a look at that piece of the code, you can see that we parse
"JSON" arguments as a Python AST and then swap out the true/false/null
literals for True/False/None. This gives some flexibility to the
syntax being entered here. I don't know off the top of my head what
happens to numbers here. This parser is only used when an argument
starts with '{' or '[', though.

This is related to another problem we have, which is that the
qom-set/qom-get scripts are pretty much extremely busted for anything
other than strings. The type safety of these interfaces is not ...
really there right now. In a beautiful future utopia, we'd be able to
get the type information we need directly from QEMU and
deterministically convert everything into the right form before
sending it out over the wire without guessing around.

--js