On Thu, Aug 8, 2024 at 6:41 PM Mauro Carvalho Chehab <
mchehab+hua...@kernel.org> wrote:
> Em Thu, 8 Aug 2024 17:21:33 -0400
> John Snow <js...@redhat.com> escreveu:
>
> > On Fri, Aug 2, 2024 at 5:44 PM Mauro Carvalho Chehab <
> > mchehab+hua...@kernel.org> wrote:
> >
>
> > > diff --git a/scripts/qmp_helper.py b/scripts/qmp_helper.py
> > > new file mode 100644
> > > index 000000000000..13fae7a7af0e
> > > --- /dev/null
> > > +++ b/scripts/qmp_helper.py
> > >
> >
> > I'm going to admit I only glanced at this very briefly, but -- is there a
> > chance you could use qemu.git/python/qemu/qmp instead of writing your own
> > helpers here?
> >
> > If *NOT*, is there something that I need to add to our QMP library to
> > facilitate your script?
>
> I started writing this script to be hosted outside qemu tree, when
> we had a very different API.
>
> I noticed later about the QMP, and even tried to write a patch for it,
> but I gave up due to asyncio complexity...
>
Sorry :)
>
> Please notice that, on this file, I actually placed three classes:
>
> - qmp
> - util
> - cper_guid
>
> I could probably make the first one to be an override of
> QEMUMonitorProtocol
> (besides normal open/close/cmd communication, it also contains some
> methods that are specific to error inject use case:
>
> - to generate a CPER record;
> - to search for data via qom-get.
>
> The other two classes are just common code used by ghes_inject commands.
> My idea is to have multiple commands to do different kinds of GHES
> error injection, each command on a different file/class.
>
Gotcha! Thanks for the feedback. I would *prefer* that code checked in to
qemu.git use the QMP module where possible so that I don't have to maintain
multiple copies of QMP wrangling code. I think what you want to do should
be easily possible with the existing library; and anything that isn't, I'm
more than happy to meet your needs. Reach out absolutely any time.
>
> > > + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> > > + try:
> > > + s.connect((host, port))
> > > + except ConnectionRefusedError:
> > > + sys.exit(f"Can't connect to QMP host {host}:{port}")
> > >
> >
> > You should be able to use e.g.
> >
> > legacy.py's QEMUMonitorProtocol class for synchronous connections, e.g.
> >
> > from qemu.qmp.legacy import QEMUMonitorProtocol
> >
> > qmp = QEMUMonitorProtocol((host, port))
> > qmp.connect(negotiate=True)
>
> That sounds interesting! I give it a try.
>
> > If you want to run the script w/o setting up a virtual environment or
> > installing the package, take a look at the hacks in scripts/qmp/ for how
> I
> > support e.g. qom-get directly from the source tree.
>
> Yeah, I saw that already. Doing:
>
> sys.path.append(path.join(qemu_dir, 'python'))
>
> the same way qom-get does should do the trick.
>
> > > +
> > > + data = s.recv(1024)
> > > + try:
> > > + obj = json.loads(data.decode("utf-8"))
> > > + except json.JSONDecodeError as e:
> > > + print(f"Invalid QMP answer: {e}")
> > > + s.close()
> > > + return
> > > +
> > > + if "QMP" not in obj:
> > > + print(f"Invalid QMP answer: {data.decode("utf-8")}")
> > > + s.close()
> > > + return
> > > +
> > > + for i, command in enumerate(commands):
> > >
> >
> > Then here you'd use qmp.cmd (raises exception on QMPError) or qmp.cmd_raw
> > or qmp.cmd_obj (returns the QMP response as the return value even if it
> was
> > an error.)
>
> Good to know, I'll try and see what fits best.
>
I might *suggest* you try to use the exception-raising interface and catch
exceptions to interrogate expected errors as it aligns better with the
"idiomatic python API" - I have no plans to support an external API that
*returns* error objects except via the exception class. This approach will
be easier to port when I drop the legacy interface in the future, see below.
But, that said, whichever is easiest. We use all three interfaces in many
places in the QEMU tree. I have no grounds to require you to use a specific
one ;)
>
> > More details:
> >
> https://qemu.readthedocs.io/projects/python-qemu-qmp/en/latest/qemu.qmp.legacy.html
>
> I'll take a look. The name "legacy" is a little scary, as it might
> imply that this has been deprecated. If there's no plans to deprecate,
> then it would be great to use it and simplify the code a little bit.
>
I named it legacy to be scary on purpose :)
The truth is that the "legacy" module was designed to be a 1:1 drop-in
replacement for an older version of the synchronous QMP library that
powered our internal iotests. We still use this "legacy" module in
thousands of places in the QEMU tree. I do have plans to replace it with a
"proper" synchronous frontend class, eventually, someday, etc. It's been a
while and I still haven't done it, though. Oops...
When I do eventually replace it, I will convert all users inside of
qemu.git personally, and the design of the "non-legacy" API will be chosen
pretty explicitly to make that task really easy for myself and reviewers.
This would include your script inside the qemu.git tree. It should be
pretty safe to use the legacy module *in qemu.git*, but for external,
out-of-tree scripts, it may indeed disappear someday - but converting to
the new API, when I merge it, should be very, very trivial. How much of a
headache that is for you depends on how you package/distribute the script
and how awful it will be to update the code and dependencies when it
happens.
FYI: I have promised in the readme for the standalone version of qemu.qmp
that legacy.py will not be removed prior to v0.1.0. All versions before
then will still have it, guaranteed.
(Neither here nor there: One of the holdups in this replacement is figuring
out how to structure the API for event listening, which was the main
motivator of the *async* version of the class. We have many users who don't
want full async handling, but still want to listen for and catch events. I
need a proper sit and think for what the API I want to commit to supporting
and maintaining for this should look like. Not your problem, anyway!)
>
> > There's also an async version, but it doesn't look like you require that
> > complexity, so you can ignore it.
>
> Yes, that's the case: a serialized sync send/response logic works perfectly
> for this script. No need to be burden with asyncio complexity.
>
> Thanks,
> Mauro
>
>
