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

[Markus Armbruster <armbru@redhat.com>]

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.


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