| Message ID | 20201026101005.2940615-1-armbru@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | qemu-storage-daemon: QAPIfy --chardev the stupid way | expand |
On 26/10/20 11:10, Markus Armbruster wrote: > Kevin's "[PATCH v2 0/6] qemu-storage-daemon: QAPIfy --chardev" > involves surgery to the QAPI generator. Some (most?) of it should go > away if we deprecate the "data" wrappers due to simple unions in QMP. > > Do we really need to mess with the code generator to solve the problem > at hand? > > > Let's recapitulate the problem: > > * We want to QAPIfy --chardev, i.e. define its argument as a QAPI > type. Considering that this is not 5.2 stuff at this point, I would like to suggest again moving chardevs to -object, and ask you to evaluate that option with the agreement that I do the work instead of you. :) Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 26/10/20 11:10, Markus Armbruster wrote: >> Kevin's "[PATCH v2 0/6] qemu-storage-daemon: QAPIfy --chardev" >> involves surgery to the QAPI generator. Some (most?) of it should go >> away if we deprecate the "data" wrappers due to simple unions in QMP. >> >> Do we really need to mess with the code generator to solve the problem >> at hand? >> >> >> Let's recapitulate the problem: >> >> * We want to QAPIfy --chardev, i.e. define its argument as a QAPI >> type. > > Considering that this is not 5.2 stuff at this point, I would like to > suggest again moving chardevs to -object, and ask you to evaluate that > option with the agreement that I do the work instead of you. :) Replacing -chardev with -object without regressing features would be lovely. One feature in particular: introspection. If we manage to fully QAPIfy -object, we should be good. I understand Eduardo is cutting a path through the jungle. I can offer assistance with bridging QAPI schema to QOM.
Am 28.10.2020 um 08:01 hat Markus Armbruster geschrieben: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 26/10/20 11:10, Markus Armbruster wrote: > >> Kevin's "[PATCH v2 0/6] qemu-storage-daemon: QAPIfy --chardev" > >> involves surgery to the QAPI generator. Some (most?) of it should go > >> away if we deprecate the "data" wrappers due to simple unions in QMP. > >> > >> Do we really need to mess with the code generator to solve the problem > >> at hand? > >> > >> > >> Let's recapitulate the problem: > >> > >> * We want to QAPIfy --chardev, i.e. define its argument as a QAPI > >> type. > > > > Considering that this is not 5.2 stuff at this point, I would like to > > suggest again moving chardevs to -object, and ask you to evaluate that > > option with the agreement that I do the work instead of you. :) I don't think this is the right thing to do at this point. Making more use of QOM is an orthogonal problem and would only make solving this one harder. The problem we have and we're trying to solve is that we have chardev-add (which has a QAPI schema) and -chardev (which doesn't). We want to get an option that is described by the schema, doesn't duplicate things and is still convenient to use. Whether this option starts with -chardev or with -object doesn't really make much of a difference. The QAPI schema you need behind it will be almost or even exactly the same. > Replacing -chardev with -object without regressing features would be > lovely. One feature in particular: introspection. > > If we manage to fully QAPIfy -object, we should be good. I understand > Eduardo is cutting a path through the jungle. I don't expect many difficulties with the existing -object (famous last words). But if you make chardevs user creatable objects first, it becomes much harder because you just combined two problems (one of which is already known to be hard) into one large problem. > I can offer assistance with bridging QAPI schema to QOM. So, the steps that I would suggest are: 1a. Finish the QAPI schema for object-add 1b. Find some way to unify chardev-add's ChardevBackend and a CLI-friendly version of it in the schema 2. Generate QOM boilerplate code from ObjectOptions instead of duplicating it in the implementation 3. Convert chardevs to using QOM properites only now If we do 3. earlier, we'll write code that we want to replace later anyway. If we even do it in parallel with 1b. we'll additionally get merge conflicts. And if we do it before 1b. we'll do it without considering QAPI in detail and will lose any flexibility to change things in the new interface, which will make 1b. much harder. Kevin
On 28/10/20 12:46, Kevin Wolf wrote: > I don't think this is the right thing to do at this point. Making more > use of QOM is an orthogonal problem and would only make solving this one > harder. Making more use of QOM will make this a non-problem. You'll just use object-add and -object and, when you figure out the QOM schema, it will just work. Paolo > The problem we have and we're trying to solve is that we have > chardev-add (which has a QAPI schema) and -chardev (which doesn't). We > want to get an option that is described by the schema, doesn't duplicate > things and is still convenient to use. > > Whether this option starts with -chardev or with -object doesn't really > make much of a difference. The QAPI schema you need behind it will be > almost or even exactly the same.
Am 28.10.2020 um 15:39 hat Paolo Bonzini geschrieben: > On 28/10/20 12:46, Kevin Wolf wrote: > > I don't think this is the right thing to do at this point. Making more > > use of QOM is an orthogonal problem and would only make solving this one > > harder. > > Making more use of QOM will make this a non-problem. You'll just use > object-add and -object and, when you figure out the QOM schema, it will > just work. Yes, but figuring out the QOM schema (or rather, what the interface represented by the schema should look like) is the hard part. It is exactly the same hard part that we're currently trying to figure out without also worrying about QOM at the same time. -object and object-add instead of -chardev and chardev-add change the spelling, but solve none of these problems. Kevin
On 28/10/20 15:59, Kevin Wolf wrote: >> Making more use of QOM will make this a non-problem. You'll just use >> object-add and -object and, when you figure out the QOM schema, it will >> just work. > > Yes, but figuring out the QOM schema (or rather, what the interface > represented by the schema should look like) is the hard part. I don't disagree with that, but it's a problem you have to solve anyway, isn't it? Once you figure out how to introspect QOM classes, that would apply just as well to character devices. On the other hand, creating character devices with -object does solve another problem, namely the distinction between "early" and "late" objects in vl.c, in a way that QAPIfied -chardev doesn't solve. Paolo
Am 28.10.2020 um 16:09 hat Paolo Bonzini geschrieben: > On 28/10/20 15:59, Kevin Wolf wrote: > >> Making more use of QOM will make this a non-problem. You'll just use > >> object-add and -object and, when you figure out the QOM schema, it will > >> just work. > > > > Yes, but figuring out the QOM schema (or rather, what the interface > > represented by the schema should look like) is the hard part. > > I don't disagree with that, but it's a problem you have to solve anyway, > isn't it? Once you figure out how to introspect QOM classes, that would > apply just as well to character devices. Yes, it's the problem I tried to address with my series, and Markus with this alternative series. We need to do this either way. > On the other hand, creating character devices with -object does solve > another problem, namely the distinction between "early" and "late" > objects in vl.c, in a way that QAPIfied -chardev doesn't solve. Right. Both are solving different problems, and solving one won't automatically make the other a non-problem as you suggested above. I just suggested leaving QOM for later because two people making big changes on the same subsystem is going to be painful for at least one of them, and because for adding QOM properties, you need to know what these properties should look like (unless you want to change them again later). If you don't wait for the QAPI work, you'll have solved the problem of having two separate ways to describe chardev options by making it three separate ways. Technically this might fulfill the condition of not having two separate ways, but it's not really what we had in mind. :-) Kevin
On 28/10/20 16:39, Kevin Wolf wrote: >> I don't disagree with that, but it's a problem you have to solve anyway, >> isn't it? Once you figure out how to introspect QOM classes, that would >> apply just as well to character devices. > Yes, it's the problem I tried to address with my series, and Markus with > this alternative series. We need to do this either way. Right, I appreciate that QOMifying chardev would only be a solution if QOM introspection gets into 6.0. This is why I only brought it up because neither of these series will be ready in time for 5.2. So maybe QOMifying chardev wouldn't make it a non-problem; it would make it someone else's (Eduardo's) problem. > you need to know what these properties should look like True that. But I think the existing QAPI structures do help for that. > If you don't wait for the QAPI work, you'll have solved the problem of > having two separate ways to describe chardev options by making it three > separate ways. Technically this might fulfill the condition of not > having two separate ways, but it's not really what we had in mind. :-) Actually four ways (-chardev, chardev-add, -object, object-add) but two of them would be implemented by the same code and qsd would be able to standardize on them. Paolo
Kevin's "[PATCH v2 0/6] qemu-storage-daemon: QAPIfy --chardev" involves surgery to the QAPI generator. Some (most?) of it should go away if we deprecate the "data" wrappers due to simple unions in QMP. Do we really need to mess with the code generator to solve the problem at hand? Let's recapitulate the problem: * We want to QAPIfy --chardev, i.e. define its argument as a QAPI type. * We want --chardev to use the same internal interface as chardev-add. * The obvious way is to reuse chardev-add's arguments type for --chardev. We don't want to, because it's excessively nested: it's a struct containing a simple union, with more simple unions inside. The result would be awful like this: --chardev id=sock0,\ backend.type=socket,\ backend.data.addr.type=inet,\ backend.data.addr.data.host=0.0.0.0,\ backend.data.addr.data.port=2445 Kevin's series solves this as follows: 1. Internal flat representation: improve the QAPI generator to represent simple unions more efficiently internally. 2. Optional external flat representation: improve the QAPI code generator and visitors to let code opt in to ditch the "data" wrappers of simple unions in the external format. Depends on 1. 3. qemu-storage-daemon --chardev opts in. This gets rid of the unwanted nesting except for "backend." 4. qemu-storage-daemon --chardev manually flattens "backend." Possible future work: 5. Accept external flat representation in addition to nested, deprecate nested. 6. After the nested representation is gone for all simple unions, simplify QAPI code generators and visitors again. 7. Only then can we drop simple unions. Note that this tackles a wider problem than just qemu-storage-daemon --chardev: the infrastructure changes support ditching "data" wrappers of simple unions *everywhere*, it just doesn't opt in elsewhere. Moreover, it provides a path towards deprecation and removal of these wrappers in QMP. A few things give me an uneasy feeling, though: * Given how close to the freeze we are, this feels awfully ambitious. * The QAPI code generator changes look okay, but they do make things more complex. If we manage to kill nested representation everywhere, most (all?) of the complexity goes away. To be frank, the "if" doesn't inspire confidence in me. Even if we pull it off, it'll probably take quite some time. * Ditching simple unions may become much harder until we kill nested representation everywhere. Right now, simple unions are a syntactical convenience. We could rewrite them to flat in the schema, and simplify the QAPI code generator. Let's take a step back and review the use of simple unions in our QAPI schema. We have just eight of them. Three occur only in command results: * query-named-block-nodes and query-block use ImageInfoSpecific * query-memory-devices uses MemoryDeviceInfo * query-tpm uses TpmTypeOptions None of them will matter for a CLI. Getting rid of "data" wrappers in results is even harder than for arguments. I doubt it's worthwhile. Five occur only in command arguments: * chardev-add and chardev-change use ChardevBackend and SocketAddressLegacy This is the problem at hand. * nbd-server-start uses SocketAddressLegacy This is a solved problem: qemu-storage-daemon --nbd-server uses SocketAddress instead. * send-key and send-event use KeyValue * transaction uses TransactionAction These are non-problems, and likely to remain non-problems forever. The --chardev is the *only* instance of the wider "simple unions make the CLI unbearably ugly" problem, as far as I can tell. What's the stupidest solution that could possibly work? The same as we used for --nbd-server: define a new, flat QAPI type. Only stupider: leave the internal interface as is, and convert the new, flat QAPI type to the old, nested one. We should really convert the other way, but the freeze makes me go for "stupidest" hard. This series does exactly that. Lightly tested. Compare to Kevin's series: * Diffstat less PATCH 1+2 (which the two have in common): mine: 8 files changed, 315 insertions(+), 18 deletions(-) *.json 1 file changed, 98 insertions(+), 8 deletions(-) *.[ch] 6 files changed, 216 insertions(+), 10 deletions(-) Kevin's: 71 files changed, 504 insertions(+), 340 deletions(-) tests/ 24 files changed, 128 insertions(+), 97 deletions(-) *.json 1 file changed, 2 insertions(+), 1 deletion(-) *.[ch] 40 files changed, 226 insertions(+), 209 deletions(-) * The bulk of my changes is straightforward and as safe as it gets: I add schema definitions, and a mostly mechanical conversion function that is only used by qemu-storage-daemon --chardev. Kevin's changes are much more complex. QAPI code generator and visitor changes potentially affect everything. As far as I can tell, they don't, and they are solid. Right now, the complexity doesn't really buy us anything. See "Possible future work" above for a few ideas on what it could buy us down the road. Tell me what you think. KEVIN Wolf (3): char/stdio: Fix QMP default for 'signal' char: Factor out qemu_chr_print_types() qemu-storage-daemon: QAPIfy --chardev Markus Armbruster (1): char: Flat alternative to overly nested chardev-add arguments qapi/char.json | 109 +++++++++++++++++++-- include/chardev/char.h | 6 ++ include/qemu/sockets.h | 3 + chardev/char-legacy.c | 140 +++++++++++++++++++++++++++ chardev/char-socket.c | 3 +- chardev/char-stdio.c | 4 +- chardev/char.c | 16 +-- storage-daemon/qemu-storage-daemon.c | 37 +++++-- util/qemu-sockets.c | 38 ++++++++ chardev/meson.build | 1 + 10 files changed, 328 insertions(+), 29 deletions(-) create mode 100644 chardev/char-legacy.c