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 :|