Re: [PATCH v3 03/12] vfio-user: define vfio-user-server object

[Stefan Hajnoczi]

On Thu, Nov 04, 2021 at 01:13:02PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > On Fri, Oct 29, 2021 at 02:42:49PM +0000, Jag Raman wrote:
> >> > On Oct 27, 2021, at 11:40 AM, Stefan Hajnoczi <stefanha@redhat.com> 
> >> > wrote:
> >> > On Mon, Oct 11, 2021 at 01:31:08AM -0400, Jagannathan Raman wrote:
> >> >> diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
> >> >> new file mode 100644
> >> >> index 0000000000..c2a300f0ff
> >> >> --- /dev/null
> >> >> +++ b/hw/remote/vfio-user-obj.c
> >> >> @@ -0,0 +1,173 @@
> >> >> +/**
> >> >> + * QEMU vfio-user-server server object
> >> >> + *
> >> >> + * Copyright © 2021 Oracle and/or its affiliates.
> >> >> + *
> >> >> + * This work is licensed under the terms of the GNU GPL-v2, version 2 
> >> >> or later.
> >> >> + *
> >> >> + * See the COPYING file in the top-level directory.
> >> >> + *
> >> >> + */
> >> >> +
> >> >> +/**
> >> >> + * Usage: add options:
> >> >> + *     -machine x-remote
> >> >> + *     -device <PCI-device>,id=<pci-dev-id>
> >> >> + *     -object vfio-user-server,id=<id>,type=unix,path=<socket-path>,
> >> > 
> >> > I expected socket.type= and socket.path= based on the QAPI schema. Is
> >> > this command-line example correct?
> >> 
> >> When I tried the “socket.path” approach, QEMU was not able to parse the
> >> arguments. So I had to break it down to a series of individual members.
> >> 
> >> If “socket.path” is the expected way, I’ll see why the parser is not 
> >> working
> >> as expected. 
> >
> > CCing Markus regarding QAPI.
> >
> > I'm surprised because the QAPI schema for vfio-user-server objects is:
> >
> >   { 'struct': 'VfioUserServerProperties',
> >     'data': { 'socket': 'SocketAddress', 'device': 'str' } }
> >
> > It's not clear to me why the command-line parser flattens the 'socket'
> > field into its 'type' and 'path' sub-fields in your example:
> >
> >   -object 
> > vfio-user-server,id=<id>,type=unix,path=<socket-path>,device=<pci-dev-id>
> >
> > Maybe because SocketAddress is an enum instead of a struct?
> >
> > Imagine a second SocketAddress field is added to vfio-user-server. How
> > can the parser know which field 'type' and 'path' belong to? I tried it:
> >
> >   { 'struct': 'VfioUserServerProperties',
> >     'data': { 'socket': 'SocketAddress', 'socket2': 'SocketAddress', 
> > 'device': 'str' } }
> >
> > Now the parser refuses any input I've tried. For example:
> >
> >   $ build/qemu-system-x86_64 -object 
> > vfio-user-server,id=s,device=asdf,type=unix,path=asdf,type=unix
> >   qemu-system-x86_64: -object 
> > vfio-user-server,id=s,device=asdf,type=unix,path=asdf,type=unix: Parameter 
> > 'type' is missing
> >
> > A similar case happens if the parent struct has 'type' or 'path' fields.
> > They collide with the SocketAddress union fields. I didn't test this
> > though.
> >
> > Questions for Markus:
> > 1. Do you know why the parser behaves like this?
> 
> Yes: backward compatibility.
> 
> The straightforward way to do a QAPI-based command line option uses
> qobject_input_visitor_new_str(), which parses either JSON or dotted
> keys, and returns the result wrapped in the appropriate QObject visitor.
> 
> The JSON syntax is derived from the QAPI schema just like for QMP.  For
> the VfioUserServerProperties shown above, it's something like
> 
>     {"socket": {"type": "unix", "path": "dir/socket"}, "device" "mumble"}
> 
> I did not check my derivation by feeding it to QEMU.  Beware of
> screwups.
> 
> The dotted keys syntax is derived from the JSON syntax as described in
> keyval.c.  For the JSON above, it should be
> 
>     socket.type=unix,socket.path=dir/socket,device=mumble
> 
> When we QAPIfy an existing option instead of adding a new QAPI-based
> one, we have an additional problem: the dotted keys syntax has to match
> the old syntax (the JSON syntax is all new, so no problem there).
> 
> The old syntax almost always has its quirks.  Ideally, we'd somehow get
> from quirky old to boring new in an orderly manner.  Sadly, we still
> don't have good solutions for that.  To make progress, we commonly
> combine JSON new with quirky old.
> 
> qemu-system-FOO -object works that way.  object_option_parse() parses
> either JSON or QemuOpts.  It wraps the former in a QObject visitor, and
> the latter in an opts visitor.
> 
> QemuOpts is flat by design[*], so the opts visitor parses flat QemuOpts
> from a (possibly non-flat) QAPI type.  How exactly it flattens, and how
> it handles clashes I don't remember.
> 
> Sadly, this means that we get quirky old even for new object types.
> 
> Questions?
> 
> > 2. Is it good practice to embed SocketAddress into parent structs or
> >    should we force it into a struct?
> 
> I'm not sure I got your question.  An example might help.

I think Question 2 isn't relevant anymore.

Thanks for the explanation!

> [*] You can play games with dotted keys to simulate nesting, but the
> opts visitor predates all that.
>

signature.asc
Description: PGP signature