| Message ID | 20220221155519.2367-5-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | qmp-shell modifications for non-interactive use | expand |
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 >
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
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. :\
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
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
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
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 > > >
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
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
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
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
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__':
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(-)