| Message ID | 20200910174850.716104-1-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | PoC: Rust binding for QAPI (qemu-ga only, for now) | expand |
marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Hi, > > Among the QEMU developers, there is a desire to use Rust. (see previous > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm > related projects and other experiments). > > Thanks to our QAPI type system and the associate code generator, it is > relatively straightforward to create Rust bindings for the generated C > types (also called sys/ffi binding) and functions. (rust-bindgen could > probably do a similar job, but it would probably bring other issues). > This provides an important internal API already. > > Slightly more complicated is to expose a Rust API for those, and provide > convenient conversions C<->Rust. Taking inspiration from glib-rs > binding, I implemented a simplified version of the FromGlib/ToGlib > traits, with simpler ownership model, sufficient for QAPI needs. > > The usage is relatively simple: > > - from_qemu_none(ptr: *const sys::P) -> T > Return a Rust type T for a const ffi pointer P. > > - from_qemu_full(ptr: *mut sys::P) -> T > Return a Rust type T for a ffi pointer P, taking ownership. > > - T::to_qemu_none() -> Stash<P> > Returns a borrowed ffi pointer P (using a Stash to destroy "glue" > storage data, if any). > > - T::to_qemu_full() -> P > Returns a ffi pointer P. (P resources are leaked/passed to C/ffi) > > With those traits, it's relatively easy to implement the QMP callbacks. > With enough interest, we could eventually start rewriting QGA in > Rust, as it is a simple service. See qga/qmp.rs for some examples. > We could also try to tackle qemu itself. Up to here, you're talking about *internal* interfaces. Correct? Your motivation is enabling use of Rust in QEMU. Correct? > Finally, given that the QAPI types are easy to serialize, it was simple > to use "serde" on them, and provide a D-Bus interface for QMP with zbus. > (a similar approach could probably be taken for other protocols, that > could be dynamically loaded... anyone like protobuf better?) QMP is an *external* interface. It supports compatible evolution: we can make certain kinds of changes without affecting clients. These include: * Adding optional arguments * Adding results * Adding values to an enumeration type, branches to a union or alternate * Reordering members of enumerations, structs, unions * Turning an argument type into an alternate with the old type as branch We've made use of this extensively. See also docs/devel/qapi-code-gen.txt section "Compatibility considerations." How do such changes affect clients of the proposed D-Bus interface? > This PoC modifies qemu-ga to provide the interface on the session bus: > $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v > $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > ... > $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1 > ... > > Note: the generated code doesn't work with the qemu schema, there is a > couple of fixme/todo left. > > Shameful pain point: meson & cargo don't play nicely together. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
On 21/09/20 11:16, Markus Armbruster wrote: > QMP is an *external* interface. > > It supports compatible evolution: we can make certain kinds of changes > without affecting clients. These include: > > * Adding optional arguments > > * Adding results > > * Adding values to an enumeration type, branches to a union or > alternate > > * Reordering members of enumerations, structs, unions > > * Turning an argument type into an alternate with the old type as branch > > We've made use of this extensively. See also > docs/devel/qapi-code-gen.txt section "Compatibility considerations." > > How do such changes affect clients of the proposed D-Bus interface? All this makes me think that Q{MP,OM,API} badly needs rationale documentation. Paolo
Hi On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote: > > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Hi, > > > > Among the QEMU developers, there is a desire to use Rust. (see previous > > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm > > related projects and other experiments). > > > > Thanks to our QAPI type system and the associate code generator, it is > > relatively straightforward to create Rust bindings for the generated C > > types (also called sys/ffi binding) and functions. (rust-bindgen could > > probably do a similar job, but it would probably bring other issues). > > This provides an important internal API already. > > > > Slightly more complicated is to expose a Rust API for those, and provide > > convenient conversions C<->Rust. Taking inspiration from glib-rs > > binding, I implemented a simplified version of the FromGlib/ToGlib > > traits, with simpler ownership model, sufficient for QAPI needs. > > > > The usage is relatively simple: > > > > - from_qemu_none(ptr: *const sys::P) -> T > > Return a Rust type T for a const ffi pointer P. > > > > - from_qemu_full(ptr: *mut sys::P) -> T > > Return a Rust type T for a ffi pointer P, taking ownership. > > > > - T::to_qemu_none() -> Stash<P> > > Returns a borrowed ffi pointer P (using a Stash to destroy "glue" > > storage data, if any). > > > > - T::to_qemu_full() -> P > > Returns a ffi pointer P. (P resources are leaked/passed to C/ffi) > > > > With those traits, it's relatively easy to implement the QMP callbacks. > > With enough interest, we could eventually start rewriting QGA in > > Rust, as it is a simple service. See qga/qmp.rs for some examples. > > We could also try to tackle qemu itself. > > Up to here, you're talking about *internal* interfaces. Correct? > > Your motivation is enabling use of Rust in QEMU. Correct? That's the first motivation, indeed. > > > Finally, given that the QAPI types are easy to serialize, it was simple > > to use "serde" on them, and provide a D-Bus interface for QMP with zbus. > > (a similar approach could probably be taken for other protocols, that > > could be dynamically loaded... anyone like protobuf better?) > > QMP is an *external* interface. > > It supports compatible evolution: we can make certain kinds of changes > without affecting clients. These include: > > * Adding optional arguments This would change the signature of the function, and would need an interface version bump. Alternative: pass optional arguments as an extra dictionary. This is a common idiom in D-Bus (the "a{sv}" type that maps strings to generic values) Potentially, use gvariant serialization format, which has maybe type. But gvariant isn't implemented by most D-Bus libraries (that was the plan long time ago, but it didn't happen as people lost interest). > * Adding results Also change the signature of the function. However, since messages have boundaries, it is easy to ignore return values. > > * Adding values to an enumeration type, branches to a union or > alternate > As long as the discriminant is represented as a string, it should be fine. > * Reordering members of enumerations, structs, unions Again, if the discriminant is a string, it should be the same as with json. For the members, the usage of dictionaries is required in this case (else the type signature would change). > * Turning an argument type into an alternate with the old type as branch That would also change the function signature. There isn't much solution I can think of, unless we have an implicit tagged enum for every argument, which would be quite nasty. > > We've made use of this extensively. See also > docs/devel/qapi-code-gen.txt section "Compatibility considerations." > > How do such changes affect clients of the proposed D-Bus interface? The introspection XML will always reflect the expected signature. You should bump your interface version whenever you make incompatible changes. If this happens too often, we could also introduce a D-Bus override mechanism to do manual translations from external interface to internal. > > > This PoC modifies qemu-ga to provide the interface on the session bus: > > $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v > > $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > > ... > > $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > > GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1 > > ... > > > > Note: the generated code doesn't work with the qemu schema, there is a > > couple of fixme/todo left. > > > > Shameful pain point: meson & cargo don't play nicely together. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >
Hi On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote: > > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Hi, > > > > Among the QEMU developers, there is a desire to use Rust. (see previous > > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm > > related projects and other experiments). > > > > Thanks to our QAPI type system and the associate code generator, it is > > relatively straightforward to create Rust bindings for the generated C > > types (also called sys/ffi binding) and functions. (rust-bindgen could > > probably do a similar job, but it would probably bring other issues). > > This provides an important internal API already. > > > > Slightly more complicated is to expose a Rust API for those, and provide > > convenient conversions C<->Rust. Taking inspiration from glib-rs > > binding, I implemented a simplified version of the FromGlib/ToGlib > > traits, with simpler ownership model, sufficient for QAPI needs. > > > > The usage is relatively simple: > > > > - from_qemu_none(ptr: *const sys::P) -> T > > Return a Rust type T for a const ffi pointer P. > > > > - from_qemu_full(ptr: *mut sys::P) -> T > > Return a Rust type T for a ffi pointer P, taking ownership. > > > > - T::to_qemu_none() -> Stash<P> > > Returns a borrowed ffi pointer P (using a Stash to destroy "glue" > > storage data, if any). > > > > - T::to_qemu_full() -> P > > Returns a ffi pointer P. (P resources are leaked/passed to C/ffi) > > > > With those traits, it's relatively easy to implement the QMP callbacks. > > With enough interest, we could eventually start rewriting QGA in > > Rust, as it is a simple service. See qga/qmp.rs for some examples. > > We could also try to tackle qemu itself. > > Up to here, you're talking about *internal* interfaces. Correct? > > Your motivation is enabling use of Rust in QEMU. Correct? > > > Finally, given that the QAPI types are easy to serialize, it was simple > > to use "serde" on them, and provide a D-Bus interface for QMP with zbus. > > (a similar approach could probably be taken for other protocols, that > > could be dynamically loaded... anyone like protobuf better?) > > QMP is an *external* interface. > > It supports compatible evolution: we can make certain kinds of changes > without affecting clients. These include: > > * Adding optional arguments > > * Adding results > > * Adding values to an enumeration type, branches to a union or > alternate > > * Reordering members of enumerations, structs, unions > > * Turning an argument type into an alternate with the old type as branch > > We've made use of this extensively. See also > docs/devel/qapi-code-gen.txt section "Compatibility considerations." > > How do such changes affect clients of the proposed D-Bus interface? > It's not just about the D-Bus interface though. QMP being JSON, being lazily typed: everytime we make such changes, we inflict some pain to all the QMP bindings that want to have a statically checked & native version of the interface. Iow, we should think twice before doing any of this. > > This PoC modifies qemu-ga to provide the interface on the session bus: > > $ qga/qemu-ga -m unix-listen -p /tmp/qga.sock -t /tmp -v > > $ busctl --user introspect org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > > ... > > $ busctl --user call org.qemu.qga /org/qemu/qga org.qemu.QgaQapi > > GuestSetVcpus aa\{sv\} 1 2 logical-id x 0 online b 1 > > ... > > > > Note: the generated code doesn't work with the qemu schema, there is a > > couple of fixme/todo left. > > > > Shameful pain point: meson & cargo don't play nicely together. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >
Paolo Bonzini <pbonzini@redhat.com> writes: > On 21/09/20 11:16, Markus Armbruster wrote: >> QMP is an *external* interface. >> >> It supports compatible evolution: we can make certain kinds of changes >> without affecting clients. These include: >> >> * Adding optional arguments >> >> * Adding results >> >> * Adding values to an enumeration type, branches to a union or >> alternate >> >> * Reordering members of enumerations, structs, unions >> >> * Turning an argument type into an alternate with the old type as branch >> >> We've made use of this extensively. See also >> docs/devel/qapi-code-gen.txt section "Compatibility considerations." >> >> How do such changes affect clients of the proposed D-Bus interface? > > All this makes me think that Q{MP,OM,API} badly needs rationale > documentation. docs/devel/qapi-code-gen.txt is fairly thorough (>8000 words), but it doesn't try to serve as rationale documentation.
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Hi > > On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> marcandre.lureau@redhat.com writes: >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> > >> > Hi, >> > >> > Among the QEMU developers, there is a desire to use Rust. (see previous >> > thread from Stefan "Why QEMU should move from C to Rust", the rust-vmm >> > related projects and other experiments). >> > >> > Thanks to our QAPI type system and the associate code generator, it is >> > relatively straightforward to create Rust bindings for the generated C >> > types (also called sys/ffi binding) and functions. (rust-bindgen could >> > probably do a similar job, but it would probably bring other issues). >> > This provides an important internal API already. >> > >> > Slightly more complicated is to expose a Rust API for those, and provide >> > convenient conversions C<->Rust. Taking inspiration from glib-rs >> > binding, I implemented a simplified version of the FromGlib/ToGlib >> > traits, with simpler ownership model, sufficient for QAPI needs. >> > >> > The usage is relatively simple: >> > >> > - from_qemu_none(ptr: *const sys::P) -> T >> > Return a Rust type T for a const ffi pointer P. >> > >> > - from_qemu_full(ptr: *mut sys::P) -> T >> > Return a Rust type T for a ffi pointer P, taking ownership. >> > >> > - T::to_qemu_none() -> Stash<P> >> > Returns a borrowed ffi pointer P (using a Stash to destroy "glue" >> > storage data, if any). >> > >> > - T::to_qemu_full() -> P >> > Returns a ffi pointer P. (P resources are leaked/passed to C/ffi) >> > >> > With those traits, it's relatively easy to implement the QMP callbacks. >> > With enough interest, we could eventually start rewriting QGA in >> > Rust, as it is a simple service. See qga/qmp.rs for some examples. >> > We could also try to tackle qemu itself. >> >> Up to here, you're talking about *internal* interfaces. Correct? >> >> Your motivation is enabling use of Rust in QEMU. Correct? > > That's the first motivation, indeed. Sounds useful. >> > Finally, given that the QAPI types are easy to serialize, it was simple >> > to use "serde" on them, and provide a D-Bus interface for QMP with zbus. >> > (a similar approach could probably be taken for other protocols, that >> > could be dynamically loaded... anyone like protobuf better?) >> >> QMP is an *external* interface. >> >> It supports compatible evolution: we can make certain kinds of changes >> without affecting clients. These include: >> >> * Adding optional arguments > > This would change the signature of the function, and would need an > interface version bump. > > Alternative: pass optional arguments as an extra dictionary. This is a > common idiom in D-Bus (the "a{sv}" type that maps strings to generic > values) > > Potentially, use gvariant serialization format, which has maybe type. > But gvariant isn't implemented by most D-Bus libraries (that was the > plan long time ago, but it didn't happen as people lost interest). > >> * Adding results > > Also change the signature of the function. > > However, since messages have boundaries, it is easy to ignore return values. I'm not sure I understand this. The compatible change I have in mind is adding members to the complex type returned by a command. >> * Adding values to an enumeration type, branches to a union or >> alternate >> > > As long as the discriminant is represented as a string, it should be fine. > >> * Reordering members of enumerations, structs, unions > > Again, if the discriminant is a string, it should be the same as with json. > > For the members, the usage of dictionaries is required in this case > (else the type signature would change). > >> * Turning an argument type into an alternate with the old type as branch > > That would also change the function signature. > > There isn't much solution I can think of, unless we have an implicit > tagged enum for every argument, which would be quite nasty. > >> >> We've made use of this extensively. See also >> docs/devel/qapi-code-gen.txt section "Compatibility considerations." >> >> How do such changes affect clients of the proposed D-Bus interface? > > The introspection XML will always reflect the expected signature. You > should bump your interface version whenever you make incompatible > changes. How do "interface versions" work? Client's and server's version need to match, or else no go? > If this happens too often, we could also introduce a D-Bus override > mechanism to do manual translations from external interface to > internal. Greek to me :)
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Hi > > On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> marcandre.lureau@redhat.com writes: [...] >> > Finally, given that the QAPI types are easy to serialize, it was simple >> > to use "serde" on them, and provide a D-Bus interface for QMP with zbus. >> > (a similar approach could probably be taken for other protocols, that >> > could be dynamically loaded... anyone like protobuf better?) >> >> QMP is an *external* interface. >> >> It supports compatible evolution: we can make certain kinds of changes >> without affecting clients. These include: >> >> * Adding optional arguments >> >> * Adding results >> >> * Adding values to an enumeration type, branches to a union or >> alternate >> >> * Reordering members of enumerations, structs, unions >> >> * Turning an argument type into an alternate with the old type as branch >> >> We've made use of this extensively. See also >> docs/devel/qapi-code-gen.txt section "Compatibility considerations." >> >> How do such changes affect clients of the proposed D-Bus interface? >> > > It's not just about the D-Bus interface though. > > QMP being JSON, being lazily typed: everytime we make such changes, we > inflict some pain to all the QMP bindings that want to have a > statically checked & native version of the interface. Iow, we should > think twice before doing any of this. Having to think twice before doing something we need to do all the time would slow us down. I don't think this is going to fly. QMP is designed to avoid tight coupling of server (i.e. QEMU) and client. In particular, we don't want "you have to upgrade both in lockstep". A well-behaved client works fine even when it's written for a somewhat older or newer QMP than the server provides. "Somewhat" because we deprecate and eventually remove stuff. Graceful degradation when the gap gets too large. There's a gap between the "lose" wire format, and a "tight" statically typed internal interface. The gap exists in QEMU, and we bridge it. Clients can do the same. Libvirt does: it provides a statically typed version of the interface without undue coupling. Replacing the "lose" wire format by something "tighter" like D-Bus shrinks the gap. Good. It also tightens the coupling. Bad. [...]
Hi On Tue, Sep 22, 2020 at 7:39 PM Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > > > Hi > > > > On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> > wrote: > >> > >> marcandre.lureau@redhat.com writes: > [...] > >> > Finally, given that the QAPI types are easy to serialize, it was > simple > >> > to use "serde" on them, and provide a D-Bus interface for QMP with > zbus. > >> > (a similar approach could probably be taken for other protocols, that > >> > could be dynamically loaded... anyone like protobuf better?) > >> > >> QMP is an *external* interface. > >> > >> It supports compatible evolution: we can make certain kinds of changes > >> without affecting clients. These include: > >> > >> * Adding optional arguments > >> > >> * Adding results > >> > >> * Adding values to an enumeration type, branches to a union or > >> alternate > >> > >> * Reordering members of enumerations, structs, unions > >> > >> * Turning an argument type into an alternate with the old type as branch > >> > >> We've made use of this extensively. See also > >> docs/devel/qapi-code-gen.txt section "Compatibility considerations." > >> > >> How do such changes affect clients of the proposed D-Bus interface? > >> > > > > It's not just about the D-Bus interface though. > > > > QMP being JSON, being lazily typed: everytime we make such changes, we > > inflict some pain to all the QMP bindings that want to have a > > statically checked & native version of the interface. Iow, we should > > think twice before doing any of this. > > Having to think twice before doing something we need to do all the time > would slow us down. I don't think this is going to fly. > > QMP is designed to avoid tight coupling of server (i.e. QEMU) and > client. In particular, we don't want "you have to upgrade both in > lockstep". > > A well-behaved client works fine even when it's written for a somewhat > older or newer QMP than the server provides. "Somewhat" because we > deprecate and eventually remove stuff. Graceful degradation when the > gap gets too large. > > There's a gap between the "lose" wire format, and a "tight" statically > typed internal interface. The gap exists in QEMU, and we bridge it. > Clients can do the same. Libvirt does: it provides a statically typed > version of the interface without undue coupling. > > Replacing the "lose" wire format by something "tighter" like D-Bus > shrinks the gap. Good. It also tightens the coupling. Bad. > > [...] > > > At least, this little D-Bus experiment puts some light on one of the current QMP weakness: it's hard to bind QMP. There are good reasons to prefer strongly typed languages. Whenever QMP is statically bound there, and such changes are made, it is pushing the versionning issues to others. It's probably one of the reasons why people are stuck binding QMP manually: doing it automatically would not be practical, as it would regularly break the interface & build. You have to version the schema & interface yourself. So we end up with multiple bindings, manually bound with mistakes etc. What does this freedom really gives us in exchange? We don't want to commit to a stable API? It's not rocket science, everybody else does it with interface version numbers. What makes QEMU/QMP so different? As for this D-Bus binding, if we don't commit to some better QMP stability guarantees, we could simply bump the version of the D-Bus interfaces for each Qemu release (without compatibility with older versions). It's not a great idea, but it's the best to do then. -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 22, 2020 at 7:39 PM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com" target="_blank">marcandre.lureau@redhat.com</a>> writes:<br> <br> > Hi<br> ><br> > On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <<a href="mailto:armbru@redhat.com" target="_blank">armbru@redhat.com</a>> wrote:<br> >><br> >> <a href="mailto:marcandre.lureau@redhat.com" target="_blank">marcandre.lureau@redhat.com</a> writes:<br> [...]<br> >> > Finally, given that the QAPI types are easy to serialize, it was simple<br> >> > to use "serde" on them, and provide a D-Bus interface for QMP with zbus.<br> >> > (a similar approach could probably be taken for other protocols, that<br> >> > could be dynamically loaded... anyone like protobuf better?)<br> >><br> >> QMP is an *external* interface.<br> >><br> >> It supports compatible evolution: we can make certain kinds of changes<br> >> without affecting clients. These include:<br> >><br> >> * Adding optional arguments<br> >><br> >> * Adding results<br> >><br> >> * Adding values to an enumeration type, branches to a union or<br> >> alternate<br> >><br> >> * Reordering members of enumerations, structs, unions<br> >><br> >> * Turning an argument type into an alternate with the old type as branch<br> >><br> >> We've made use of this extensively. See also<br> >> docs/devel/qapi-code-gen.txt section "Compatibility considerations."<br> >><br> >> How do such changes affect clients of the proposed D-Bus interface?<br> >><br> ><br> > It's not just about the D-Bus interface though.<br> ><br> > QMP being JSON, being lazily typed: everytime we make such changes, we<br> > inflict some pain to all the QMP bindings that want to have a<br> > statically checked & native version of the interface. Iow, we should<br> > think twice before doing any of this.<br> <br> Having to think twice before doing something we need to do all the time<br> would slow us down. I don't think this is going to fly.<br> <br> QMP is designed to avoid tight coupling of server (i.e. QEMU) and<br> client. In particular, we don't want "you have to upgrade both in<br> lockstep".<br> <br> A well-behaved client works fine even when it's written for a somewhat<br> older or newer QMP than the server provides. "Somewhat" because we<br> deprecate and eventually remove stuff. Graceful degradation when the<br> gap gets too large.<br> <br> There's a gap between the "lose" wire format, and a "tight" statically<br> typed internal interface. The gap exists in QEMU, and we bridge it.<br> Clients can do the same. Libvirt does: it provides a statically typed<br> version of the interface without undue coupling.<br> <br> Replacing the "lose" wire format by something "tighter" like D-Bus<br> shrinks the gap. Good. It also tightens the coupling. Bad.<br> <br> [...]<br> <br> <br> </blockquote></div><div><br></div><div><br></div><div>At least, this little D-Bus experiment puts some light on one of the current QMP weakness: it's hard to bind QMP.<br><br>There are good reasons to prefer strongly typed languages. Whenever QMP is statically bound there, and such changes are made, it is pushing the versionning issues to others. It's probably one of the reasons why people are stuck binding QMP manually: doing it automatically would not be practical, as it would regularly break the interface & build. You have to version the schema & interface yourself. <br><br>So we end up with multiple bindings, manually bound with mistakes etc.<br><br>What does this freedom really gives us in exchange? We don't want to commit to a stable API? It's not rocket science, everybody else does it with interface version numbers. What makes QEMU/QMP so different?<br><br>As for this D-Bus binding, if we don't commit to some better QMP stability guarantees, we could simply bump the version of the D-Bus interfaces for each Qemu release (without compatibility with older versions). It's not a great idea, but it's the best to do then.</div><div><br></div><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
Hi On Tue, Sep 22, 2020 at 8:11 PM Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > > >> * Adding results > > > > Also change the signature of the function. > > > > However, since messages have boundaries, it is easy to ignore return > values. > > I'm not sure I understand this. > > The compatible change I have in mind is adding members to the complex > type returned by a command. > There are certain things you can do in D-Bus, just like we do with JSON. You could ignore the signature checking, you could ignore some extra message fields... That's not what people usually expect. D-Bus is a machine and bindings friendly. It's not meant to be so lazy. > >> We've made use of this extensively. See also > >> docs/devel/qapi-code-gen.txt section "Compatibility considerations." > >> > >> How do such changes affect clients of the proposed D-Bus interface? > > > > The introspection XML will always reflect the expected signature. You > > should bump your interface version whenever you make incompatible > > changes. > > How do "interface versions" work? Client's and server's version need to > match, or else no go? > > The D-Bus specification doesn't detail versioning much. What is recommended is to have the version number as part of the interface name (kinda like soname): http://0pointer.de/blog/projects/versioning-dbus.html (this is documented in several places iirc) So a QEMU D-Bus interface could have a name like org.qemu.Qemu51, org.qemu.Qemu52.. for example, if we can't provide better API stability... -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 22, 2020 at 8:11 PM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Marc-André Lureau <<a href="mailto:marcandre.lureau@redhat.com" target="_blank">marcandre.lureau@redhat.com</a>> writes:<br> <br> >> * Adding results<br> ><br> > Also change the signature of the function.<br> ><br> > However, since messages have boundaries, it is easy to ignore return values.<br> <br> I'm not sure I understand this.<br> <br> The compatible change I have in mind is adding members to the complex<br> type returned by a command.<br></blockquote><div><br></div><div><br></div><div>There are certain things you can do in D-Bus, just like we do with JSON. You could ignore the signature checking, you could ignore some extra message fields... That's not what people usually expect. D-Bus is a machine and bindings friendly. It's not meant to be so lazy.<br></div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> >> We've made use of this extensively. See also<br> >> docs/devel/qapi-code-gen.txt section "Compatibility considerations."<br> >><br> >> How do such changes affect clients of the proposed D-Bus interface?<br> ><br> > The introspection XML will always reflect the expected signature. You<br> > should bump your interface version whenever you make incompatible<br> > changes.<br> <br> How do "interface versions" work? Client's and server's version need to<br> match, or else no go?<br> <br clear="all"></blockquote><div><br></div><div>The D-Bus specification doesn't detail versioning much. What is recommended is to have the version number as part of the interface name (kinda like soname): <a href="http://0pointer.de/blog/projects/versioning-dbus.html">http://0pointer.de/blog/projects/versioning-dbus.html</a> (this is documented in several places iirc)</div><div><br></div><div>So a QEMU D-Bus interface could have a name like org.qemu.Qemu51, org.qemu.Qemu52.. for example, if we can't provide better API stability...<br></div></div><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On Tue, Sep 22, 2020 at 05:09:56PM +0200, Markus Armbruster wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > > > Hi > > > > On Mon, Sep 21, 2020 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote: > >> > >> We've made use of this extensively. See also > >> docs/devel/qapi-code-gen.txt section "Compatibility considerations." > >> > >> How do such changes affect clients of the proposed D-Bus interface? > > > > The introspection XML will always reflect the expected signature. You > > should bump your interface version whenever you make incompatible > > changes. > > How do "interface versions" work? Client's and server's version need to > match, or else no go? Yes, both client and server need to support the same "interface" in order to talk to each other. If a service makes an incompatible API change, then then change the interface name. It is common to include a number in the interface name. Of course some changes can be made in a backwards compatible manner so don't require changing the interface. eg adding new methods, adding new signals (aka async events), adding new properties. Adding/changing/removing parameters in an existing method is what causes an interface incompatability. You can potentially mitigate the inability to change parameters by modelling parameters in a variety of ways. Many of the tricks common in traditional C libraries for future proofing API designs will apply equally well to DBus APIs. For example, instead of modelling everything as a distinct positional parameter, you can model them as optional named parameters. These would be exposed as an array(map(string-variant)) - basically think of it as a hash table. This is a half-way house between very strongly typed and very weakly typed worlds. Libvirt takes this approach with some of our C APIs that we expect to grow parameters over time. A variant on this is to expose some data in a custom document format as a single parameter. For example if there was an API requesting a description of a block backend properties, instead of having a DBus API where all the block backend props were explicitly modelled, just have a single string parameter that returns a JSON/XML doc. You can extend that doc at will without impacting the DBus API. Again libvirt takes this approach with our XML doc schema. A final useful technique is to have a generic "int flags" parameter as a way to express new functional behaviour by defining extra flags. The challenge with all these ideas is figuring out whether there's a viable way to map them into how we describe the current QMP protocol. I don't think there's a especially good answer to do a 100% automated mapping from QMP to DBus, while keeping extensibility and also maximising strong type checking benefits. There's a tradeoff between extensibility and strong typing that needs some intelligent thought on a case by case basis IMHO. For an automated mapping, either we go fully strong typed and accept that we'll be breaking DBus interface compatibility continually, or we go full weakly typed and accept that clients won't get much type validation benefits at build time. It is the inherant problem with using something weakly typed as the master specification and trying to translate it to something strongly typed. 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 :|
On 22/09/20 18:35, Marc-André Lureau wrote: > The D-Bus specification doesn't detail versioning much. What is > recommended is to have the version number as part of the interface name > (kinda like soname): > http://0pointer.de/blog/projects/versioning-dbus.html (this is > documented in several places iirc) > > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51, > org.qemu.Qemu52.. for example, if we can't provide better API stability... That would be a problem for backports. It seems to me that the bindings issue is only a problem if we insist on having positional arguments like we do for C, but if we can avoid functions with a zillion arguments we could. For example in Rust, it's idiomatic to use the builder pattern let thread = thread::Builder::new() .name("foo".into()) .stack_size(65536) .spawn(run_thread)?; thread.join()?; and I think the same would work in Go or even C++. It would look like qapi::qga::commands::GuestShutdown::new() .mode("halt") .invoke_on(qapi_channel)?; with some kind of double dispatch implementation: trait QAPIChannel { ... fn invoke(command: dyn QAPISerialization) -> dyn QAPISerialization; } impl GuestShutdown { fn<T: QAPIChannel> invoke_on(t: T) -> () { let args = self.as_qapi_serialization(); t.invoke(args); // could "return from_qapi_serialization(result)", likewise } } In Python, you can use keyword arguments and there are even keyword-only arguments ("def f(*, key1, key2)"), like qapi.qga.GuestFileOpen(path="/etc/passwd").invoke_on(qapi_channel); When you do something like this QMP-style APIs are not a problem. FlatBuffers is another serialization format that supports this kind of extensibility (https://google.github.io/flatbuffers/ explicitly compares it to JSON, even). Paolo
Daniel P. Berrangé <berrange@redhat.com> writes: [...] > The challenge with all these ideas is figuring out whether there's > a viable way to map them into how we describe the current QMP protocol. > I don't think there's a especially good answer to do a 100% automated > mapping from QMP to DBus, while keeping extensibility and also maximising > strong type checking benefits. There's a tradeoff between extensibility > and strong typing that needs some intelligent thought on a case by case > basis IMHO. > > For an automated mapping, either we go fully strong typed and accept > that we'll be breaking DBus interface compatibility continually, or > we go full weakly typed and accept that clients won't get much type > validation benefits at build time. > > It is the inherant problem with using something weakly typed as the > master specification and trying to translate it to something strongly > typed. I avoid "weakly typed" and "strongly typed", because it means different things to different people. "Statically typed" and "dynamically typed" are clear concepts. Interfaces defined with QAPI are statically typed. Serialization / deserialization via QMP works even when the interfaces on both ends differ in certain limited ways. It supports more differences than an RPC transport whose idea of "procedure" has both feet in the grave of ALGOL 60 could. At either end of the wire, we need to bridge the gap between a programming language and the wire format. At the server end (QEMU), we use the QAPI code generator to bridge from C, and for the one specific version of the QAPI-defined interface this build of QEMU provides. Clients we've left to fend for themselves. Perhaps it's been a matter of not understanding client requirements well enough, and chickening out. A client could do the same as QEMU does: bridge one specific version of the QAPI-defined interface. Could even be done with a suitably extended QAPI code generator. A client could also bridge multiple interface versions, if that us useful. Regardless of the number of versions bridged, the client could still talk to servers that use another version, thanks to the QMP transport. When the server (i.e. QEMU) is newer, any new stuff it may provide is inaccessible (d'oh). Also, old stuff may be gone (rare, involves going through the deprecation process). Graceful degradation when the client lags behind too much. When the server is older than the client, any new stuff the client actually uses won't work. The client can use introspection to adjust itself to older servers. Graceful degradation again. Nothing so far precludes a statically typed interface for client code! The problem isn't "weakly typed" QMP (whatever that may mean), it's that chasing the evolving QAPI-defined interface with manually written code is a lot of work. Libvirt has been doing that work, but you know, "what have the Romans ever done for us?" Marc-André's D-Bus experiment explores another way to bridge the gap. This bridge is generated, not hand-written. Because the QMP transport also gets replaced by one that lacks QMP's capability to "mediate" between versions, client and server versions become tightly coupled. This isn't due to static typing. It's due to use of a tool that makes different tradeoffs than we made with QAPI/QMP. I'm not fundamentally opposed to adding some QAPI/not-QMP external interface, but I'd prefer not to end up with the union of disadvantages and the intersection of advantages. If QMP no longer matches our requirements, we should replace it wholesale.
Marc-André Lureau <marcandre.lureau@gmail.com> writes: [...] > What does this freedom really gives us in exchange? We don't want to commit > to a stable API? It's not rocket science, everybody else does it with > interface version numbers. What makes QEMU/QMP so different? It's not rocket science, and we're so used to it that we don't even notice anymore how awful it is. When you compile to native code, exact interface match is required for efficiency. This used to be pretty much a non-issue: when you compile and link statically, the only interface remaining at run time is system calls. Dynamic linking threw us into DLL hell. Yes, we figured out how to version symbols, when to bump sonames, and how prepare for and make binary compatible interface changes. It's still awful. People deploy in containers just to get out of this awful game. But remember: there's a *reason*, namely efficiency. Once you go beyond a single process, you need interprocess communication. We use procedure calls for intraprocess communication, so remote procedure calls are an obvious solution for interprocess communication. Where many RPC systems have gone wrong, in my opinion, is bringing along the awfulness of exact interface matches, with much less of a reason, but even more awfulness: you now get to also wrestle with multiple versions of servers fighting over ports and such. Yes, containers, I know. They help a lot with keeping such messes under control. But some messes are necessary, while others are not. I respectfully disagree with the notion that "everybody else does it with interface version numbers". There's a ton of IPC protocols out there that do not require exact interface matches. [...]
Hi On Tue, Sep 22, 2020 at 9:08 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 22/09/20 18:35, Marc-André Lureau wrote: > > The D-Bus specification doesn't detail versioning much. What is > > recommended is to have the version number as part of the interface name > > (kinda like soname): > > http://0pointer.de/blog/projects/versioning-dbus.html (this is > > documented in several places iirc) > > > > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51, > > org.qemu.Qemu52.. for example, if we can't provide better API > stability... > > That would be a problem for backports. > > Yes.. Backports could expose a subset of the new version interface? Or the interface can be divided for each Qmp command. (unorthodox) It seems to me that the bindings issue is only a problem if we insist on > having positional arguments like we do for C, but if we can avoid > functions with a zillion arguments we could. For example in Rust, it's > idiomatic to use the builder pattern > > let thread = thread::Builder::new() > .name("foo".into()) > .stack_size(65536) > .spawn(run_thread)?; > thread.join()?; > > and I think the same would work in Go or even C++. It would look like > > qapi::qga::commands::GuestShutdown::new() > .mode("halt") > .invoke_on(qapi_channel)?; > > Or simply use the same approach as qapi-rs ( https://github.com/arcnmx/qapi-rs) which is simply generating data structures based on the schema, and not binding commands to Rust functions for ex. qga.execute(&qga::guest_shutdown { mode: Some(GuestShutdownMode::Halt) }) Less idiomatic, but it also works around the optional arguments and ordering issue. In both cases, the client interface should be versionized (since some fields may be added or becoming optional, return value may appear etc), and only at runtime can you actually verify what is actually supported. In Python, you can use keyword arguments and there are even keyword-only > arguments ("def f(*, key1, key2)"), like > > qapi.qga.GuestFileOpen(path="/etc/passwd").invoke_on(qapi_channel); > > Yes, the python binding will have a similar issue. And if we want to add typing to the mix, representing everything as a dict is not going to help much. Fortunately, there are other options I believe. But I would rather aim for the obvious, having non-optional & ordered arguments, and interface/methods versioning. When you do something like this QMP-style APIs are not a problem. > FlatBuffers is another serialization format that supports this kind of > extensibility (https://google.github.io/flatbuffers/ explicitly compares > it to JSON, even). > Well, I wouldn't say it's not a problem. It makes working with QMP as a client quite an unpleasant experience overall imho... -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 22, 2020 at 9:08 PM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com">pbonzini@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 22/09/20 18:35, Marc-André Lureau wrote:<br> > The D-Bus specification doesn't detail versioning much. What is<br> > recommended is to have the version number as part of the interface name<br> > (kinda like soname):<br> > <a href="http://0pointer.de/blog/projects/versioning-dbus.html" rel="noreferrer" target="_blank">http://0pointer.de/blog/projects/versioning-dbus.html</a> (this is<br> > documented in several places iirc)<br> > <br> > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51,<br> > org.qemu.Qemu52.. for example, if we can't provide better API stability...<br> <br> That would be a problem for backports.<br> <br></blockquote><div><br></div><div>Yes.. Backports could expose a subset of the new version interface? Or the interface can be divided for each Qmp command. (unorthodox)<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> It seems to me that the bindings issue is only a problem if we insist on<br> having positional arguments like we do for C, but if we can avoid<br> functions with a zillion arguments we could. For example in Rust, it's<br> idiomatic to use the builder pattern<br> <br> let thread = thread::Builder::new()<br> .name("foo".into())<br> .stack_size(65536)<br> .spawn(run_thread)?;<br> thread.join()?;<br> <br> and I think the same would work in Go or even C++. It would look like<br> <br> qapi::qga::commands::GuestShutdown::new()<br> .mode("halt")<br> .invoke_on(qapi_channel)?;<br> <br></blockquote><div><br></div><div>Or simply use the same approach as qapi-rs (<a href="https://github.com/arcnmx/qapi-rs">https://github.com/arcnmx/qapi-rs</a>) which is simply generating data structures based on the schema, and not binding commands to Rust functions for ex.</div><div><br></div><div>qga.execute(&qga::guest_shutdown { mode: Some(GuestShutdownMode::Halt) })</div><div><br></div><div>Less idiomatic, but it also works around the optional arguments and ordering issue.<br></div><div><br></div><div>In both cases, the client interface should be versionized (since some fields may be added or becoming optional, return value may appear etc), and only at runtime can you actually verify what is actually supported.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">In Python, you can use keyword arguments and there are even keyword-only<br> arguments ("def f(*, key1, key2)"), like<br> <br> qapi.qga.GuestFileOpen(path="/etc/passwd").invoke_on(qapi_channel);<br> <br></blockquote><div><br></div><div>Yes, the python binding will have a similar issue. And if we want to add typing to the mix, representing everything as a dict is not going to help much. Fortunately, there are other options I believe. But I would rather aim for the obvious, having non-optional & ordered arguments, and interface/methods versioning.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> When you do something like this QMP-style APIs are not a problem.<br> FlatBuffers is another serialization format that supports this kind of<br> extensibility (<a href="https://google.github.io/flatbuffers/" rel="noreferrer" target="_blank">https://google.github.io/flatbuffers/</a> explicitly compares<br> it to JSON, even).<br></blockquote><div><br></div><div>Well, I wouldn't say it's not a problem. It makes working with QMP as a client quite an unpleasant experience overall imho...<br></div></div><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On 29/09/20 09:45, Marc-André Lureau wrote: > Hi > > On Tue, Sep 22, 2020 at 9:08 PM Paolo Bonzini <pbonzini@redhat.com > <mailto:pbonzini@redhat.com>> wrote: > > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51, > > org.qemu.Qemu52.. for example, if we can't provide better API > stability... > > That would be a problem for backports. > > Yes.. Backports could expose a subset of the new version interface? Or > the interface can be divided for each Qmp command. (unorthodox) That seems like a workaround for an IDL that is not the right solution for the problem. > qapi::qga::commands::GuestShutdown::new() > .mode("halt") > .invoke_on(qapi_channel)?; > > > Or simply use the same approach as qapi-rs > (https://github.com/arcnmx/qapi-rs) which is simply generating data > structures based on the schema, and not binding commands to Rust > functions for ex. > > qga.execute(&qga::guest_shutdown { mode: Some(GuestShutdownMode::Halt) }) That would not be backwards compatible as you would have to set all optional fields. Every time the command grows a new optional argument, all clients would have to specify it; if a command becomes optional, you'd have to add Some() around it. So I would say that... > Less idiomatic, but it also works around the optional arguments and > ordering issue. ... the builder pattern is not a workaround: it's the best and most common Rust idiom to achieve what QAPI expresses as optional fields. Likewise for keyword-only arguments in Python. > Yes, the python binding will have a similar issue. And if we want to add > typing to the mix, representing everything as a dict is not going to > help much. Fortunately, there are other options I believe. But I would > rather aim for the obvious, having non-optional & ordered arguments, and > interface/methods versioning. You shouldn't just state what you want; you really should take a look at how the ability to add optional arguments has been used in the past, and see if an alternative RPC without optional and unordered arguments would have been as effective. D-Bus is probably a perfectly good API for qemu-ga. The experience with qemu-ga however does not necessarily extend to QEMU. The main issue with D-Bus is that it conflates the transport and the IDL so that you have to resort to passing arguments as key-value pairs. QMP does the same, but the IDL is much more flexible and not QEMU-specific, so we don't pay as high a price. Remember that while the specific transport of QMP ("JSON dictionaries over a socket with 'execute' and 'arguments' keys") is QEMU-specific, the concept of using JSON payloads for RPC is very common. For example, REST APIs almost invariably use JSON and the resulting API even "feel" somewhat similar to QMP. In fact, The first Google result for "openapi backwards compatible extensions" (https://github.com/zalando/restful-api-guidelines/blob/master/chapters/compatibility.adoc#107) might as well be written for QMP: * [server] Add only optional, never mandatory fields. * [client] Be tolerant with unknown fields in the payload * [server] Unknown input fields in payload or URL should not be ignored * [client] API clients consuming data must not assume that objects are closed for extension * [server] When changing your RESTful APIs, do so in a compatible way and avoid generating additional API versions * [server] MUST not use URI versioning and so on. If you want to "reinvent" QMP, instead of focusing on D-Bus you should take a look at alternative IDLs and protocols (D-Bus is one but there's also Protobuf and Flexbuffers), see how QAPI declarations would map to those protocols, see how you would deal with extensibility, and rank them according to various criteria. For example: * JSON "just works" but needs a custom code generator and imposes some extra complexity on the clients for the simplest commands * D-Bus has a good ecosystem and would keep simple commands simpler but has issues with upgrade paths and is uglier for complex commands * Protobufs probably would also just work and would have better code generators, but would require some kind of lint to ensure backwards-compatibility * etc. > Well, I wouldn't say it's not a problem. It makes working with QMP as a > client quite an unpleasant experience overall imho... With automatically-generated bindings, the experience writing a QMP client is as pleasant as the code generator makes it. Paolo
Hi On Tue, Sep 29, 2020 at 2:15 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 29/09/20 09:45, Marc-André Lureau wrote: > > Hi > > > > On Tue, Sep 22, 2020 at 9:08 PM Paolo Bonzini <pbonzini@redhat.com > > <mailto:pbonzini@redhat.com>> wrote: > > > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51, > > > org.qemu.Qemu52.. for example, if we can't provide better API > > stability... > > > > That would be a problem for backports. > > > > Yes.. Backports could expose a subset of the new version interface? Or > > the interface can be divided for each Qmp command. (unorthodox) > > That seems like a workaround for an IDL that is not the right solution > for the problem. > > > qapi::qga::commands::GuestShutdown::new() > > .mode("halt") > > .invoke_on(qapi_channel)?; > > > > > > Or simply use the same approach as qapi-rs > > (https://github.com/arcnmx/qapi-rs) which is simply generating data > > structures based on the schema, and not binding commands to Rust > > functions for ex. > > > > qga.execute(&qga::guest_shutdown { mode: Some(GuestShutdownMode::Halt) }) > > > That would not be backwards compatible as you would have to set all > optional fields. Every time the command grows a new optional argument, > all clients would have to specify it; if a command becomes optional, > you'd have to add Some() around it. So I would say that... > > Not necessarily, with ..default::Default() > Less idiomatic, but it also works around the optional arguments and > > ordering issue. > > ... the builder pattern is not a workaround: it's the best and most > common Rust idiom to achieve what QAPI expresses as optional fields. > Likewise for keyword-only arguments in Python. > Except QAPI makes all fields potentially optional (and unordered), that's not idiomatic. > > Yes, the python binding will have a similar issue. And if we want to add > > typing to the mix, representing everything as a dict is not going to > > help much. Fortunately, there are other options I believe. But I would > > rather aim for the obvious, having non-optional & ordered arguments, and > > interface/methods versioning. > > You shouldn't just state what you want; you really should take a look at > how the ability to add optional arguments has been used in the past, and > see if an alternative RPC without optional and unordered arguments would > have been as effective. D-Bus is probably a perfectly good API for > qemu-ga. The experience with qemu-ga however does not necessarily > extend to QEMU. > > The main issue with D-Bus is that it conflates the transport and the IDL > so that you have to resort to passing arguments as key-value pairs. QMP > D-Bus is machine-level oriented, it's easy to bind to various languages, it can be pretty efficient too. It's not designed to be a good network RPC. QMP tries to be a bit of both, but is perhaps not good enough in either. does the same, but the IDL is much more flexible and not QEMU-specific, > so we don't pay as high a price. Remember that while the specific > transport of QMP ("JSON dictionaries over a socket with 'execute' and > 'arguments' keys") is QEMU-specific, the concept of using JSON payloads > for RPC is very common. For example, REST APIs almost invariably use > JSON and the resulting API even "feel" somewhat similar to QMP. > > In fact, The first Google result for "openapi backwards compatible > extensions" > ( > https://github.com/zalando/restful-api-guidelines/blob/master/chapters/compatibility.adoc#107 > ) > might as well be written for QMP: > > * [server] Add only optional, never mandatory fields. > > * [client] Be tolerant with unknown fields in the payload > > * [server] Unknown input fields in payload or URL should not be ignored > > * [client] API clients consuming data must not assume that objects are > closed for extension > > * [server] When changing your RESTful APIs, do so in a compatible way > and avoid generating additional API versions > > * [server] MUST not use URI versioning > > and so on. > > If you want to "reinvent" QMP, instead of focusing on D-Bus you should > take a look at alternative IDLs and protocols (D-Bus is one but there's > also Protobuf and Flexbuffers), see how QAPI declarations would map to > those protocols, see how you would deal with extensibility, and rank > them according to various criteria. For example: > > * JSON "just works" but needs a custom code generator and imposes some > extra complexity on the clients for the simplest commands > > * D-Bus has a good ecosystem and would keep simple commands simpler but > has issues with upgrade paths and is uglier for complex commands > > * Protobufs probably would also just work and would have better code > generators, but would require some kind of lint to ensure > backwards-compatibility > > Again, the issues we are discussing are not specific to binding QMP over D-Bus. Binding QMP to various languages has similar problems. Perhaps those problems are minor, perhaps we can find decent solutions, like the one you suggest to use Rust builder pattern for commands. I think they are not great, as I tried to explain with the versioning and runtime issues that you have to take into account anyway at the client level. I would rather make those problems solved at the server level, that doesn't require any change to QMP today, just a more careful consideration when making changes (and probably some tools to help enforce some stability). -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 29, 2020 at 2:15 PM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com">pbonzini@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 29/09/20 09:45, Marc-André Lureau wrote:<br> > Hi<br> > <br> > On Tue, Sep 22, 2020 at 9:08 PM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com" target="_blank">pbonzini@redhat.com</a><br> > <mailto:<a href="mailto:pbonzini@redhat.com" target="_blank">pbonzini@redhat.com</a>>> wrote:<br> > > So a QEMU D-Bus interface could have a name like org.qemu.Qemu51,<br> > > org.qemu.Qemu52.. for example, if we can't provide better API<br> > stability...<br> > <br> > That would be a problem for backports.<br> > <br> > Yes.. Backports could expose a subset of the new version interface? Or<br> > the interface can be divided for each Qmp command. (unorthodox)<br> <br> That seems like a workaround for an IDL that is not the right solution<br> for the problem.<br> <br> > qapi::qga::commands::GuestShutdown::new()<br> > .mode("halt")<br> > .invoke_on(qapi_channel)?;<br> > <br> > <br> > Or simply use the same approach as qapi-rs<br> > (<a href="https://github.com/arcnmx/qapi-rs" rel="noreferrer" target="_blank">https://github.com/arcnmx/qapi-rs</a>) which is simply generating data<br> > structures based on the schema, and not binding commands to Rust<br> > functions for ex.<br> > <br> > qga.execute(&qga::guest_shutdown { mode: Some(GuestShutdownMode::Halt) })<br> <br> <br> That would not be backwards compatible as you would have to set all<br> optional fields. Every time the command grows a new optional argument,<br> all clients would have to specify it; if a command becomes optional,<br> you'd have to add Some() around it. So I would say that...<br> <br></blockquote><div><br></div><div>Not necessarily, with ..default::Default()</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > Less idiomatic, but it also works around the optional arguments and<br> > ordering issue.<br> <br> ... the builder pattern is not a workaround: it's the best and most<br> common Rust idiom to achieve what QAPI expresses as optional fields.<br> Likewise for keyword-only arguments in Python.<br></blockquote><div><br></div><div>Except QAPI makes all fields potentially optional (and unordered), that's not idiomatic.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > Yes, the python binding will have a similar issue. And if we want to add<br> > typing to the mix, representing everything as a dict is not going to<br> > help much. Fortunately, there are other options I believe. But I would<br> > rather aim for the obvious, having non-optional & ordered arguments, and<br> > interface/methods versioning.<br> <br> You shouldn't just state what you want; you really should take a look at<br> how the ability to add optional arguments has been used in the past, and<br> see if an alternative RPC without optional and unordered arguments would<br> have been as effective. D-Bus is probably a perfectly good API for<br> qemu-ga. The experience with qemu-ga however does not necessarily<br> extend to QEMU.<br> <br> The main issue with D-Bus is that it conflates the transport and the IDL<br> so that you have to resort to passing arguments as key-value pairs. QMP<br></blockquote><div><br></div><div>D-Bus is machine-level oriented, it's easy to bind to various languages, it can be pretty efficient too. It's not designed to be a good network RPC. QMP tries to be a bit of both, but is perhaps not good enough in either.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> does the same, but the IDL is much more flexible and not QEMU-specific,<br> so we don't pay as high a price. Remember that while the specific<br> transport of QMP ("JSON dictionaries over a socket with 'execute' and<br> 'arguments' keys") is QEMU-specific, the concept of using JSON payloads<br> for RPC is very common. For example, REST APIs almost invariably use<br> JSON and the resulting API even "feel" somewhat similar to QMP.<br> <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> In fact, The first Google result for "openapi backwards compatible<br> extensions"<br> (<a href="https://github.com/zalando/restful-api-guidelines/blob/master/chapters/compatibility.adoc#107" rel="noreferrer" target="_blank">https://github.com/zalando/restful-api-guidelines/blob/master/chapters/compatibility.adoc#107</a>)<br> might as well be written for QMP:<br> <br> * [server] Add only optional, never mandatory fields.<br> <br> * [client] Be tolerant with unknown fields in the payload<br> <br> * [server] Unknown input fields in payload or URL should not be ignored<br> <br> * [client] API clients consuming data must not assume that objects are<br> closed for extension<br> <br> * [server] When changing your RESTful APIs, do so in a compatible way<br> and avoid generating additional API versions<br> <br> * [server] MUST not use URI versioning<br> <br> and so on.<br> <br> If you want to "reinvent" QMP, instead of focusing on D-Bus you should<br> take a look at alternative IDLs and protocols (D-Bus is one but there's<br> also Protobuf and Flexbuffers), see how QAPI declarations would map to<br> those protocols, see how you would deal with extensibility, and rank<br> them according to various criteria. For example:<br> <br> * JSON "just works" but needs a custom code generator and imposes some<br> extra complexity on the clients for the simplest commands<br> <br> * D-Bus has a good ecosystem and would keep simple commands simpler but<br> has issues with upgrade paths and is uglier for complex commands<br> <br> * Protobufs probably would also just work and would have better code<br> generators, but would require some kind of lint to ensure<br> backwards-compatibility<br> <br> </blockquote><div><br></div><div><div><br></div><div>Again, the issues we are discussing are not specific to binding QMP over D-Bus. Binding QMP to various languages has similar problems. Perhaps those problems are minor, perhaps we can find decent solutions, like the one you suggest to use Rust builder pattern for commands. I think they are not great, as I tried to explain with the versioning and runtime issues that you have to take into account anyway at the client level. I would rather make those problems solved at the server level, that doesn't require any change to QMP today, just a more careful consideration when making changes (and probably some tools to help enforce some stability).<br></div></div><div><br></div></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On 29/09/20 12:34, Marc-André Lureau wrote: > That would not be backwards compatible as you would have to set all > optional fields. Every time the command grows a new optional argument, > all clients would have to specify it; if a command becomes optional, > you'd have to add Some() around it. So I would say that... > > > Not necessarily, with ..default::Default() That's true, I always forget about .. (though you'd still have to add Some() for now-optional fields). > > Less idiomatic, but it also works around the optional arguments and > > ordering issue. > > ... the builder pattern is not a workaround: it's the best and most > common Rust idiom to achieve what QAPI expresses as optional fields. > Likewise for keyword-only arguments in Python. > > Except QAPI makes all fields potentially optional (and unordered), > that's not idiomatic. Yes, for some APIs you can always add hand-written, more idiomatic versions. Or you could mark them as fixed-arguments in the schema and let the code generator do that (but then you need to add a compatibility check). But that would be an explicit choice, not something required by the transport. > D-Bus is machine-level oriented, it's easy to bind to various languages, > it can be pretty efficient too. It's not designed to be a good network > RPC. QMP tries to be a bit of both, but is perhaps not good enough in > either. No, only tries to be a good network RPC; not a particularly efficient one, but future-proof. And it mostly succeeds at that---with one notable exception: JSON parsers that mess up with numbers bigger than 2^53. > If you want to "reinvent" QMP, instead of focusing on D-Bus you should > take a look at alternative IDLs and protocols (D-Bus is one but there's > also Protobuf and Flexbuffers), see how QAPI declarations would map to > those protocols, see how you would deal with extensibility, and rank > them according to various criteria. For example: > > * JSON "just works" but needs a custom code generator and imposes some > extra complexity on the clients for the simplest commands > > * D-Bus has a good ecosystem and would keep simple commands simpler but > has issues with upgrade paths and is uglier for complex commands > > * Protobufs probably would also just work and would have better code > generators, but would require some kind of lint to ensure > backwards-compatibility > > Again, the issues we are discussing are not specific to binding QMP over > D-Bus. Binding QMP to various languages has similar problems. Marc-André, we are totally in agreement about that! The problem is that you have already decided what the solution looks like, and that's what I'm not sure about because your solution also implies completely revisiting the schema. I say there are many candidates (the ones I know are Protobuf and Flexbuffers) for serialization and many candidates for transport (REST and gRPC to begin with) in addition to the two {QMP,JSON} and {DBus,DBus} tuples. We should at least look at how they do code generation before deciding that JSON is bad and DBus is good. > I would rather make those problems solved at the server level, that > doesn't require any change to QMP today, just a more careful > consideration when making changes (and probably some tools to help > enforce some stability). Problem is, "more careful consideration when making changes" is not a small thing. And other RPCs have evolved in a completely different space (REST APIs for web services) that have chosen the same tradeoffs as QMP, so why should we not learn from them? Paolo
Hi On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 29/09/20 12:34, Marc-André Lureau wrote: > > That would not be backwards compatible as you would have to set all > > optional fields. Every time the command grows a new optional > argument, > > all clients would have to specify it; if a command becomes optional, > > you'd have to add Some() around it. So I would say that... > > > > > > Not necessarily, with ..default::Default() > > That's true, I always forget about .. (though you'd still have to add > Some() for now-optional fields). > > > > Less idiomatic, but it also works around the optional arguments and > > > ordering issue. > > > > ... the builder pattern is not a workaround: it's the best and most > > common Rust idiom to achieve what QAPI expresses as optional fields. > > Likewise for keyword-only arguments in Python. > > > > Except QAPI makes all fields potentially optional (and unordered), > > that's not idiomatic. > > Yes, for some APIs you can always add hand-written, more idiomatic > versions. Or you could mark them as fixed-arguments in the schema and > let the code generator do that (but then you need to add a compatibility > check). But that would be an explicit choice, not something required by > the transport. > That's my point, if we agree on keeping arguments & members non-optional and ordered, we are doing 50% of the job for nicer automated bindings. Second half would probably be involving some version information in the schema. > > D-Bus is machine-level oriented, it's easy to bind to various languages, > > it can be pretty efficient too. It's not designed to be a good network > > RPC. QMP tries to be a bit of both, but is perhaps not good enough in > > either. > > No, only tries to be a good network RPC; not a particularly efficient > one, but future-proof. And it mostly succeeds at that---with one > notable exception: JSON parsers that mess up with numbers bigger than 2^53. > > > If you want to "reinvent" QMP, instead of focusing on D-Bus you > should > > take a look at alternative IDLs and protocols (D-Bus is one but > there's > > also Protobuf and Flexbuffers), see how QAPI declarations would map > to > > those protocols, see how you would deal with extensibility, and rank > > them according to various criteria. For example: > > > > * JSON "just works" but needs a custom code generator and imposes > some > > extra complexity on the clients for the simplest commands > > > > * D-Bus has a good ecosystem and would keep simple commands simpler > but > > has issues with upgrade paths and is uglier for complex commands > > > > * Protobufs probably would also just work and would have better code > > generators, but would require some kind of lint to ensure > > backwards-compatibility > > > > Again, the issues we are discussing are not specific to binding QMP over > > D-Bus. Binding QMP to various languages has similar problems. > > Marc-André, we are totally in agreement about that! The problem is that > you have already decided what the solution looks like, and that's what > I'm not sure about because your solution also implies completely > revisiting the schema. > Did I? Which schema change are you (or I) implying? Versioning the interface? It's necessary at the client level, unless everything is dynamic, after introspection, which makes automated static bindings impractical. > I say there are many candidates (the ones I know are Protobuf and > Flexbuffers) for serialization and many candidates for transport (REST > and gRPC to begin with) in addition to the two {QMP,JSON} and > {DBus,DBus} tuples. We should at least look at how they do code > generation before deciding that JSON is bad and DBus is good. > Contrary to what you believe I am not focusing so much on DBus here :) It took about 200 loc to bind it, effortlessly (compared to sys<->rs conversion). All it does is to expose the same API we have in the generated C somehow (similar static types & functions - not all as a{sv} opaque dictionaries). It's easy for QEMU to generate a good static binding for C, because the version always matches. For a client, you wouldn't be able to write a similar idiomatic API in C, Rust, Python or Go, unfortunately. Iow, I am not trying to sell DBus, I would like to make it easier to bind QMP in general. (although I do believe that DBus is a better protocol than QMP for local IPC, yes. And gRPC is probably better for remoting) > I would rather make those problems solved at the server level, that > > doesn't require any change to QMP today, just a more careful > > consideration when making changes (and probably some tools to help > > enforce some stability). > > Problem is, "more careful consideration when making changes" is not a > small thing. And other RPCs have evolved in a completely different > space (REST APIs for web services) that have chosen the same tradeoffs > as QMP, so why should we not learn from them? > > I don't buy that generalization. A very recent protocol in this space, that aims to be a good low-level RPC on Linux (for containers, cloud etc) is varlink. (In many ways, we could compare it to QMP, but it lacks some important features, like events) varlink does non-optional members and versioning the same way I propose here, for what I could tell. Although they use JSON, and have similar transport "benefits", this basic rule allow them to have very decent automated binding in various languages, without resorting to unorthodox solutions, ex: https://github.com/varlink/rust/blob/master/examples/example/src/main.rs -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com">pbonzini@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 29/09/20 12:34, Marc-André Lureau wrote:<br> > That would not be backwards compatible as you would have to set all<br> > optional fields. Every time the command grows a new optional argument,<br> > all clients would have to specify it; if a command becomes optional,<br> > you'd have to add Some() around it. So I would say that...<br> > <br> > <br> > Not necessarily, with ..default::Default()<br> <br> That's true, I always forget about .. (though you'd still have to add<br> Some() for now-optional fields).<br> <br> > > Less idiomatic, but it also works around the optional arguments and<br> > > ordering issue.<br> > <br> > ... the builder pattern is not a workaround: it's the best and most<br> > common Rust idiom to achieve what QAPI expresses as optional fields.<br> > Likewise for keyword-only arguments in Python.<br> > <br> > Except QAPI makes all fields potentially optional (and unordered),<br> > that's not idiomatic.<br> <br> Yes, for some APIs you can always add hand-written, more idiomatic<br> versions. Or you could mark them as fixed-arguments in the schema and<br> let the code generator do that (but then you need to add a compatibility<br> check). But that would be an explicit choice, not something required by<br> the transport.<br></blockquote><div><br></div><div>That's my point, if we agree on keeping arguments & members non-optional and ordered, we are doing 50% of the job for nicer automated bindings.</div></div><div class="gmail_quote"><br></div><div class="gmail_quote">Second half would probably be involving some version information in the schema.<br></div><div class="gmail_quote"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > D-Bus is machine-level oriented, it's easy to bind to various languages,<br> > it can be pretty efficient too. It's not designed to be a good network<br> > RPC. QMP tries to be a bit of both, but is perhaps not good enough in<br> > either.<br> <br> No, only tries to be a good network RPC; not a particularly efficient<br> one, but future-proof. And it mostly succeeds at that---with one<br> notable exception: JSON parsers that mess up with numbers bigger than 2^53.<br> <br> > If you want to "reinvent" QMP, instead of focusing on D-Bus you should<br> > take a look at alternative IDLs and protocols (D-Bus is one but there's<br> > also Protobuf and Flexbuffers), see how QAPI declarations would map to<br> > those protocols, see how you would deal with extensibility, and rank<br> > them according to various criteria. For example:<br> > <br> > * JSON "just works" but needs a custom code generator and imposes some<br> > extra complexity on the clients for the simplest commands<br> > <br> > * D-Bus has a good ecosystem and would keep simple commands simpler but<br> > has issues with upgrade paths and is uglier for complex commands<br> > <br> > * Protobufs probably would also just work and would have better code<br> > generators, but would require some kind of lint to ensure<br> > backwards-compatibility<br> > <br> > Again, the issues we are discussing are not specific to binding QMP over<br> > D-Bus. Binding QMP to various languages has similar problems.<br> <br> Marc-André, we are totally in agreement about that! The problem is that<br> you have already decided what the solution looks like, and that's what<br> I'm not sure about because your solution also implies completely<br> revisiting the schema.<br></blockquote><div><br></div><div>Did I? Which schema change are you (or I) implying? Versioning the interface? It's necessary at the client level, unless everything is dynamic, after introspection, which makes automated static bindings impractical.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> I say there are many candidates (the ones I know are Protobuf and<br> Flexbuffers) for serialization and many candidates for transport (REST<br> and gRPC to begin with) in addition to the two {QMP,JSON} and<br> {DBus,DBus} tuples. We should at least look at how they do code<br> generation before deciding that JSON is bad and DBus is good.<br></blockquote><div> </div><div>Contrary to what you believe I am not focusing so much on DBus here :) It took about 200 loc to bind it, effortlessly (compared to sys<->rs conversion). All it does is to expose the same API we have in the generated C somehow (similar static types & functions - not all as a{sv} opaque dictionaries). <br></div><div><br></div><div>It's easy for QEMU to generate a good static binding for C, because the version always matches. For a client, you wouldn't be able to write a similar idiomatic API in C, Rust, Python or Go, unfortunately. <br></div><div><br></div><div>Iow, I am not trying to sell DBus, I would like to make it easier to bind QMP in general. (although I do believe that DBus is a better protocol than QMP for local IPC, yes. And gRPC is probably better for remoting)<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > I would rather make those problems solved at the server level, that<br> > doesn't require any change to QMP today, just a more careful<br> > consideration when making changes (and probably some tools to help<br> > enforce some stability).<br> <br> Problem is, "more careful consideration when making changes" is not a<br> small thing. And other RPCs have evolved in a completely different<br> space (REST APIs for web services) that have chosen the same tradeoffs<br> as QMP, so why should we not learn from them?<br> <br></blockquote><div><br></div>I don't buy that generalization. A very recent protocol in this space, that aims to be a good low-level RPC on Linux (for containers, cloud etc) is varlink. (In many ways, we could compare it to QMP, but it lacks some important features, like events)</div><div class="gmail_quote"><br></div><div class="gmail_quote">varlink does non-optional members and versioning the same way I propose here, for what I could tell. Although they use JSON, and have similar transport "benefits", this basic rule allow them to have very decent automated binding in various languages, without resorting to unorthodox solutions, ex: <a href="https://github.com/varlink/rust/blob/master/examples/example/src/main.rs">https://github.com/varlink/rust/blob/master/examples/example/src/main.rs</a></div><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
Hi On Fri, Sep 11, 2020 at 7:17 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 11/09/20 16:00, Marc-André Lureau wrote: > > - from_qemu_none should be a "to_*" or "constructor" conversion (I > used > > new_from_foreign) > > > > new_ prefix is not very rusty. > > Right, I have changed it to with_foreign so now there is > {as,with,to,into}_foreign, plus unsafe_{from,into}. > > These two could even be renamed to from_foreign and into_native at the > cost of making the trait less general purpose. This way we have the > typical Rust names: as_* for Borrowed->Borrowed, with_*/to_* for > Borrowed->Owned, from_*/into_* for Owned->Owned. > > > However, the memory allocator (or the stack) may not be compatible > > with the one used in C. > > Hmm, that's a good point. The simplest solution might be just to get > rid of IntoForeign, it's just an optimization. > > > from_raw() is common, and takes ownership. > > from_raw()/into_raw() would be equivalent to > into_foreign()/from_foreign(). However as you point out the allocators > are different, so it's a good idea IMHO to separate > into_raw()/from_raw() for the Rust allocator from > into_foreign()/from_foreign() for the libc allocator. > > > I would need to modify this PoC for example > > Yes of course. Can you just try splitting the PoC in multiple patches? > That should also make it easier to review, so far all I did was > comparing against glib-rs. > > > But I must say I feel quite comfortable with the glib approach. It > > would be nice to have some feedback from glib-rs maintainers about your > > proposal. > > QAPI is not tied to glib-rs, so I don't think qemu-ga will need to use > glib-rs. I think either we use glib-rs, or if we are to roll our own we > should not be tied to the naming. We don't use GObject introspection, > so none/full means nothing to most QEMU developers (and to Rust > developers too). > > There are other things I don't like very much in glib-rs, for example > the use of tuples and public fields and the somewhat messy usage of > *const/*mut (I tried to be stricter on that). > > I am trying to wrap my head around your proposal (based on https://github.com/bonzini/rust-ptr), and trying to understand the limitations/unrustiness of the glib-rs translate traits I used in this PoC. First let's clarify the requirements. We need those conversions for now: - const *P -> T - mut *P -> T And: - &T -> const *P - &T -> mut *P Note that glib-rs has more advanced conversions, because of partial ownership transfer with containers, and ref-counted types etc. Those could soon become necessary for QEMU to bind other types than QAPI, in particular QOM and our usage of glib in general. I kept that in mind by carefully choosing glib-rs as a reference. I think it's important to take it into account from the start (sadly, some limitations don't allow us to simply use glib-rs traits, for reasons that aren't 100% clear to me, but are clear to the compiler and others :) Some other remarks: - "mut *P -> T" is often just "const *P -> T" with P being freed after conversion - "&T -> const *P" can be "&T -> mut *P" with Rust side freeing P after usage thanks to a stash, but can also be very different and not require it (strings for example, the constP uses CString, while the mutP version is just a g_strndup) - it is nice (or necessary) to have to allow some form of composition for container-like types (Option<T>, Vec<T>, struct T(U,V) inside etc) to avoid duplication - Rust naming conventions guide us towards using to_ and into_ (for owned->owned) prefixes. The glib-rs traits map the conversion functions respectively to (I removed the Glib/Qemu prefix, because the subset used in both are very close): - FromPtrNone::from_none - FromPtrFull::from_full (usually just calls from_none() and free(P)) And: - ToPtr::to_none (with the Stash) - ToPtr::to_full The symmetry is clear, and arguably easy to remember. fwiw, I don't know why ToPtr wasn't split the same way FromPtr was (they used to be on the same FromPtr trait). The usage of to_ prefix is in accordance with the Rust conventions here. The usage of from_ is perhaps not ideal?, but from_full is not incompatible with the symmetrical into_ (as in From<T> for U implies Into<U> for T). Experience shows that the combination of Stash & ToPtr design makes it convenient for type composition too. My understanding of what you propose is: - ForeignConvert::with_foreign - FromForeign::from_foreign (with implied into_native) And: - ForeignConvert::as_foreign (with the BorrowedPointer/stash-like) - ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems wrongly designed in your proposal and unnecessary for now) I excluded IntoForeign::into_foreign, since "T -> P" can't really be done better than "&T -> *P" due to different memory allocators etc. I don't have your head, so I find it hard to remember & work with. It uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_. That just blows my mind, sorry :) Then, I don't understand why ForeignConvert should hold both the "const *P -> T" and "&T -> const *P" conversions. Except the common types, what's the relation between the two? Finally, I thought you introduced some differences with the stash design, but in fact I can see that ForeignConvert::Storage works just the way as ToPtr::Storage. So composition should be similar. Only your example code is more repetitive as it doesn't indirectly refer to the trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage). I am not making any conclusions yet, but I am not exactly happily going to switch to your proposal yet :) Comments? -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 11, 2020 at 7:17 PM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com">pbonzini@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 11/09/20 16:00, Marc-André Lureau wrote:<br> > - from_qemu_none should be a "to_*" or "constructor" conversion (I used<br> > new_from_foreign)<br> > <br> > new_ prefix is not very rusty.<br> <br> Right, I have changed it to with_foreign so now there is<br> {as,with,to,into}_foreign, plus unsafe_{from,into}.<br> <br> These two could even be renamed to from_foreign and into_native at the<br> cost of making the trait less general purpose. This way we have the<br> typical Rust names: as_* for Borrowed->Borrowed, with_*/to_* for<br> Borrowed->Owned, from_*/into_* for Owned->Owned.<br> <br> > However, the memory allocator (or the stack) may not be compatible<br> > with the one used in C.<br> <br> Hmm, that's a good point. The simplest solution might be just to get<br> rid of IntoForeign, it's just an optimization.<br> <br> > from_raw() is common, and takes ownership.<br> <br> from_raw()/into_raw() would be equivalent to<br> into_foreign()/from_foreign(). However as you point out the allocators<br> are different, so it's a good idea IMHO to separate<br> into_raw()/from_raw() for the Rust allocator from<br> into_foreign()/from_foreign() for the libc allocator.<br> <br> > I would need to modify this PoC for example<br> <br> Yes of course. Can you just try splitting the PoC in multiple patches?<br> That should also make it easier to review, so far all I did was<br> comparing against glib-rs.<br> <br> > But I must say I feel quite comfortable with the glib approach. It<br> > would be nice to have some feedback from glib-rs maintainers about your<br> > proposal.<br> <br> QAPI is not tied to glib-rs, so I don't think qemu-ga will need to use<br> glib-rs. I think either we use glib-rs, or if we are to roll our own we<br> should not be tied to the naming. We don't use GObject introspection,<br> so none/full means nothing to most QEMU developers (and to Rust<br> developers too).<br> <br> There are other things I don't like very much in glib-rs, for example<br> the use of tuples and public fields and the somewhat messy usage of<br> *const/*mut (I tried to be stricter on that).<br> <br></blockquote><div><br></div><div>I am trying to wrap my head around your proposal (based on <a href="https://github.com/bonzini/rust-ptr">https://github.com/bonzini/rust-ptr</a>), and trying to understand the limitations/unrustiness of the glib-rs translate traits I used in this PoC.<br><br>First let's clarify the requirements. We need those conversions for now:<br>- const *P -> T<br>- mut *P -> T<br>And:<br>- &T -> const *P<br>- &T -> mut *P<br><br>Note that glib-rs has more advanced conversions, because of partial ownership transfer with containers, and ref-counted types etc. Those could soon become necessary for QEMU to bind other types than QAPI, in particular QOM and our usage of glib in general. I kept that in mind by carefully choosing glib-rs as a reference. I think it's important to take it into account from the start (sadly, some limitations don't allow us to simply use glib-rs traits, for reasons that aren't 100% clear to me, but are clear to the compiler and others :)<br><br>Some other remarks:<br>- "mut *P -> T" is often just "const *P -> T" with P being freed after conversion<br>- "&T -> const *P" can be "&T -> mut *P" with Rust side freeing P after usage thanks to a stash, but can also be very different and not require it (strings for example, the constP uses CString, while the mutP version is just a g_strndup)<br>- it is nice (or necessary) to have to allow some form of composition for container-like types (Option<T>, Vec<T>, struct T(U,V) inside etc) to avoid duplication<br>- Rust naming conventions guide us towards using to_ and into_ (for owned->owned) prefixes.<br><br><br>The glib-rs traits map the conversion functions respectively to (I removed the Glib/Qemu prefix, because the subset used in both are very close):<br>- FromPtrNone::from_none<br>- FromPtrFull::from_full (usually just calls from_none() and free(P))<br>And:<br>- ToPtr::to_none (with the Stash)<br>- ToPtr::to_full<br><br>The symmetry is clear, and arguably easy to remember. fwiw, I don't know why ToPtr wasn't split the same way FromPtr was (they used to be on the same FromPtr trait).<br><br>The usage of to_ prefix is in accordance with the Rust conventions here. The usage of from_ is perhaps not ideal?, but from_full is not incompatible with the symmetrical into_ (as in From<T> for U implies Into<U> for T).<br><br>Experience shows that the combination of Stash & ToPtr design makes it convenient for type composition too.<br><br><br>My understanding of what you propose is:<br>- ForeignConvert::with_foreign<br>- FromForeign::from_foreign (with implied into_native)<br>And:<br>- ForeignConvert::as_foreign (with the BorrowedPointer/stash-like)<br>- ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems wrongly designed in your proposal and unnecessary for now)<br><br>I excluded IntoForeign::into_foreign, since "T -> P" can't really be done better than "&T -> *P" due to different memory allocators etc.<br><br>I don't have your head, so I find it hard to remember & work with. It uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_. That just blows my mind, sorry :)<br><br>Then, I don't understand why ForeignConvert should hold both the "const *P -> T" and "&T -> const *P" conversions. Except the common types, what's the relation between the two?<br><br>Finally, I thought you introduced some differences with the stash design, but in fact I can see that ForeignConvert::Storage works just the way as ToPtr::Storage. So composition should be similar. Only your example code is more repetitive as it doesn't indirectly refer to the trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage).<br><br>I am not making any conclusions yet, but I am not exactly happily going to switch to your proposal yet :)<br><br>Comments?<br><br><br><br><br> </div></div><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On 29/09/20 19:55, Marc-André Lureau wrote: > My understanding of what you propose is: > - ForeignConvert::with_foreign > - FromForeign::from_foreign (with implied into_native) > And: > - ForeignConvert::as_foreign (with the BorrowedPointer/stash-like) > - ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems > wrongly designed in your proposal and unnecessary for now) Might well be, but how is it wrong? (I'd like to improve). > I don't have your head, so I find it hard to remember & work with. It> uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_. > That just blows my mind, sorry :) Ahah I don't have your head either! The idea anyway is to reuse prefixes that are common in Rust code: * with_: a constructor that uses something to build a type (think Vec::with_capacity) and therefore takes ownership * as_: a cheap conversion to something, it's cheap because it reuses the lifetime (and therefore takes no ownership). Think Option::as_ref. * from_/to_: a copying and possibly expensive conversion (that you have to write the code for). Because it's copying, it doesn't consume the argument (for from_) or self (for to_). * into_: a conversion that consumes the receiver It may well be over the top. > Then, I don't understand why ForeignConvert should hold both the "const > *P -> T" and "&T -> const *P" conversions. Except the common types, > what's the relation between the two? Maybe I'm wrong, but why would you need just one? > Finally, I thought you introduced some differences with the stash > design, but in fact I can see that ForeignConvert::Storage works just > the way as ToPtr::Storage. So composition should be similar. Only your > example code is more repetitive as it doesn't indirectly refer to the > trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage). Yes, that's the main difference. I removed Storage because I didn't want to force any trait on BorrowedPointer's second type argument. It seemed like a generic concept to me. The other difference is that Stash is a tuple while BorrowedPointer is a struct and has methods to access it. Stash seems very ugly to use. > I am not making any conclusions yet, but I am not exactly happily going > to switch to your proposal yet :) Sure, no problem. Paolo
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com> wrote: [...] >> Marc-André, we are totally in agreement about that! The problem is that >> you have already decided what the solution looks like, and that's what >> I'm not sure about because your solution also implies completely >> revisiting the schema. >> > > Did I? Which schema change are you (or I) implying? Versioning the > interface? It's necessary at the client level, unless everything is > dynamic, after introspection, which makes automated static bindings > impractical. I disagree with "necessary". A client can use a specific version of QMP, and still talk to a server with a different version, because we designed that capability into QMP. You absolutely can create bindings for a specific version of QMP for the client if you want. Just make sure the client as a whole obeys the rules of the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt. >> I say there are many candidates (the ones I know are Protobuf and >> Flexbuffers) for serialization and many candidates for transport (REST >> and gRPC to begin with) in addition to the two {QMP,JSON} and >> {DBus,DBus} tuples. We should at least look at how they do code >> generation before deciding that JSON is bad and DBus is good. >> > > Contrary to what you believe I am not focusing so much on DBus here :) It > took about 200 loc to bind it, effortlessly (compared to sys<->rs > conversion). All it does is to expose the same API we have in the generated > C somehow (similar static types & functions - not all as a{sv} opaque > dictionaries). Two points. 1. Opaque dictionaries are far from the only way to do keyword arguments in a language that lacks them. 2. The API we generate for C is not exactly wonderful. Behold this beauty: void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, bool has_base_node, const char *base_node, bool has_base, const char *base, bool has_top_node, const char *top_node, bool has_top, const char *top, bool has_backing_file, const char *backing_file, bool has_speed, int64_t speed, bool has_on_error, BlockdevOnError on_error, bool has_filter_node_name, const char *filter_node_name, bool has_auto_finalize, bool auto_finalize, bool has_auto_dismiss, bool auto_dismiss, Error **errp); It's gotten so bad we added a second way to do the C API: void qmp_drive_backup(DriveBackup *arg, Error **errp); Turns out DriveBackup arg = { ... initialize the optionals you need ... } qmp_drive_backup(&arg, &err); is a lot easier on the eyes than passing 29 positional arguments. This could be viewed as a work-around for C's lack of positional parameters. Even more fun: void qmp_blockdev_add(BlockdevOptions *arg, Error **errp); BlockdevOptions is a tagged union. This could be viewed as a work-around for C's lack of function overloading. > It's easy for QEMU to generate a good static binding for C, because the > version always matches. For a client, you wouldn't be able to write a > similar idiomatic API in C, Rust, Python or Go, unfortunately. I disagree. You won't be able to write good bindings using just positional parameters. Not even if you add restrictions on how we can evolve QMP. And no, I do not consider the C bindings we create for QEMU itself "good". They're the best we could do, and good enough. When you do bindings for another language, do bindings for that language, not C bindings in that language. Regardless of bindings, the client as a whole should obey the rules of the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt. If these rules have become counter-productive, then it's time to replace QMP wholesale. Do not attempt to force a square peg into a round hole. If we must have square pegs, design a square hole, and retire the round hole. > Iow, I am not trying to sell DBus, I would like to make it easier to bind > QMP in general. (although I do believe that DBus is a better protocol than > QMP for local IPC, yes. And gRPC is probably better for remoting) > >> I would rather make those problems solved at the server level, that >> > doesn't require any change to QMP today, just a more careful >> > consideration when making changes (and probably some tools to help >> > enforce some stability). >> >> Problem is, "more careful consideration when making changes" is not a >> small thing. And other RPCs have evolved in a completely different >> space (REST APIs for web services) that have chosen the same tradeoffs >> as QMP, so why should we not learn from them? >> >> > I don't buy that generalization. A very recent protocol in this space, that > aims to be a good low-level RPC on Linux (for containers, cloud etc) is > varlink. (In many ways, we could compare it to QMP, but it lacks some > important features, like events) > > varlink does non-optional members and versioning the same way I propose > here, for what I could tell. Although they use JSON, and have similar > transport "benefits", this basic rule allow them to have very decent > automated binding in various languages, without resorting to unorthodox > solutions, ex: > https://github.com/varlink/rust/blob/master/examples/example/src/main.rs Paolo pointed out successful protocols that make tradeoffs similar to QMP to support the idea that these tradeoffs can make sense and are workable. Pointing out other, dissimilar protocols is not a convincing counter-argument :)
Hi On Wed, Sep 30, 2020 at 11:34 AM Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > > Hi > > > > On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com> > wrote: > [...] > >> Marc-André, we are totally in agreement about that! The problem is that > >> you have already decided what the solution looks like, and that's what > >> I'm not sure about because your solution also implies completely > >> revisiting the schema. > >> > > > > Did I? Which schema change are you (or I) implying? Versioning the > > interface? It's necessary at the client level, unless everything is > > dynamic, after introspection, which makes automated static bindings > > impractical. > > I disagree with "necessary". > > A client can use a specific version of QMP, and still talk to a server > with a different version, because we designed that capability into QMP. > "A client can use a specific version of QMP" == versioning on the client side > > You absolutely can create bindings for a specific version of QMP for the > client if you want. Just make sure the client as a whole obeys the > rules of the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt. > > >> I say there are many candidates (the ones I know are Protobuf and > >> Flexbuffers) for serialization and many candidates for transport (REST > >> and gRPC to begin with) in addition to the two {QMP,JSON} and > >> {DBus,DBus} tuples. We should at least look at how they do code > >> generation before deciding that JSON is bad and DBus is good. > >> > > > > Contrary to what you believe I am not focusing so much on DBus here :) It > > took about 200 loc to bind it, effortlessly (compared to sys<->rs > > conversion). All it does is to expose the same API we have in the > generated > > C somehow (similar static types & functions - not all as a{sv} opaque > > dictionaries). > > Two points. > > 1. Opaque dictionaries are far from the only way to do keyword arguments > in a language that lacks them. > Oh one can always be creative. The point is trying to stay idiomatic in the target language. > > 2. The API we generate for C is not exactly wonderful. > > Behold this beauty: > > void qmp_block_commit(bool has_job_id, const char *job_id, const char > *device, bool has_base_node, const char *base_node, bool has_base, const > char *base, bool has_top_node, const char *top_node, bool has_top, const > char *top, bool has_backing_file, const char *backing_file, bool has_speed, > int64_t speed, bool has_on_error, BlockdevOnError on_error, bool > has_filter_node_name, const char *filter_node_name, bool has_auto_finalize, > bool auto_finalize, bool has_auto_dismiss, bool auto_dismiss, Error **errp); > > It's gotten so bad we added a second way to do the C API: > > void qmp_drive_backup(DriveBackup *arg, Error **errp); > > Turns out > > DriveBackup arg = { > ... initialize the optionals you need ... > } > qmp_drive_backup(&arg, &err); > > is a lot easier on the eyes than passing 29 positional arguments. > > So is writing the function arguments with indentation. Then I don't see much difference between a long list of arguments in a struct and that function. The main difference is that you make it easy to pass those arguments down. But often, you want to pass a subset, you don't want to pass the whole context as it may lead to bad design / bugs. This could be viewed as a work-around for C's lack of positional > parameters. > > Or a badly designed QMP command. Even more fun: > > void qmp_blockdev_add(BlockdevOptions *arg, Error **errp); > > BlockdevOptions is a tagged union. > > This could be viewed as a work-around for C's lack of function > overloading. > > Or a badly designed QMP command ? > It's easy for QEMU to generate a good static binding for C, because the > > version always matches. For a client, you wouldn't be able to write a > > similar idiomatic API in C, Rust, Python or Go, unfortunately. > > I disagree. You won't be able to write good bindings using just > positional parameters. Not even if you add restrictions on how we can > evolve QMP. And no, I do not consider the C bindings we create for QEMU > itself "good". They're the best we could do, and good enough. > > Sure they could be better, they are still quite idiomatic for C. When you do bindings for another language, do bindings for that > language, not C bindings in that language. > > Yes Regardless of bindings, the client as a whole should obey the rules of > the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt. If these > rules have become counter-productive, then it's time to replace QMP > wholesale. > > Do not attempt to force a square peg into a round hole. If we must have > square pegs, design a square hole, and retire the round hole. > > Hmm? I am trying to make the hole a bit more regular... > Iow, I am not trying to sell DBus, I would like to make it easier to bind > > QMP in general. (although I do believe that DBus is a better protocol > than > > QMP for local IPC, yes. And gRPC is probably better for remoting) > > > >> I would rather make those problems solved at the server level, that > >> > doesn't require any change to QMP today, just a more careful > >> > consideration when making changes (and probably some tools to help > >> > enforce some stability). > >> > >> Problem is, "more careful consideration when making changes" is not a > >> small thing. And other RPCs have evolved in a completely different > >> space (REST APIs for web services) that have chosen the same tradeoffs > >> as QMP, so why should we not learn from them? > >> > >> > > I don't buy that generalization. A very recent protocol in this space, > that > > aims to be a good low-level RPC on Linux (for containers, cloud etc) is > > varlink. (In many ways, we could compare it to QMP, but it lacks some > > important features, like events) > > > > varlink does non-optional members and versioning the same way I propose > > here, for what I could tell. Although they use JSON, and have similar > > transport "benefits", this basic rule allow them to have very decent > > automated binding in various languages, without resorting to unorthodox > > solutions, ex: > > https://github.com/varlink/rust/blob/master/examples/example/src/main.rs > > Paolo pointed out successful protocols that make tradeoffs similar to > QMP to support the idea that these tradeoffs can make sense and are > workable. > > Pointing out other, dissimilar protocols is not a convincing > counter-argument :) > It's relevant. Did you study varlink a bit? It's so close to QMP, you will find it hard to point out real dissimilarities. -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 30, 2020 at 11:34 AM Markus Armbruster <<a href="mailto:armbru@redhat.com">armbru@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Marc-André Lureau <<a href="mailto:marcandre.lureau@gmail.com" target="_blank">marcandre.lureau@gmail.com</a>> writes:<br> <br> > Hi<br> ><br> > On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com" target="_blank">pbonzini@redhat.com</a>> wrote:<br> [...]<br> >> Marc-André, we are totally in agreement about that! The problem is that<br> >> you have already decided what the solution looks like, and that's what<br> >> I'm not sure about because your solution also implies completely<br> >> revisiting the schema.<br> >><br> ><br> > Did I? Which schema change are you (or I) implying? Versioning the<br> > interface? It's necessary at the client level, unless everything is<br> > dynamic, after introspection, which makes automated static bindings<br> > impractical.<br> <br> I disagree with "necessary".<br> <br> A client can use a specific version of QMP, and still talk to a server<br> with a different version, because we designed that capability into QMP.<br></blockquote><div><br></div><div> "A client can use a specific version of QMP" == versioning on the client side<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> You absolutely can create bindings for a specific version of QMP for the<br> client if you want. Just make sure the client as a whole obeys the<br> rules of the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt.<br> <br> >> I say there are many candidates (the ones I know are Protobuf and<br> >> Flexbuffers) for serialization and many candidates for transport (REST<br> >> and gRPC to begin with) in addition to the two {QMP,JSON} and<br> >> {DBus,DBus} tuples. We should at least look at how they do code<br> >> generation before deciding that JSON is bad and DBus is good.<br> >><br> ><br> > Contrary to what you believe I am not focusing so much on DBus here :) It<br> > took about 200 loc to bind it, effortlessly (compared to sys<->rs<br> > conversion). All it does is to expose the same API we have in the generated<br> > C somehow (similar static types & functions - not all as a{sv} opaque<br> > dictionaries).<br> <br> Two points.<br> <br> 1. Opaque dictionaries are far from the only way to do keyword arguments<br> in a language that lacks them.<br></blockquote><div><br></div><div>Oh one can always be creative. The point is trying to stay idiomatic in the target language.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> 2. The API we generate for C is not exactly wonderful.<br> <br> Behold this beauty:<br> <br> void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, bool has_base_node, const char *base_node, bool has_base, const char *base, bool has_top_node, const char *top_node, bool has_top, const char *top, bool has_backing_file, const char *backing_file, bool has_speed, int64_t speed, bool has_on_error, BlockdevOnError on_error, bool has_filter_node_name, const char *filter_node_name, bool has_auto_finalize, bool auto_finalize, bool has_auto_dismiss, bool auto_dismiss, Error **errp);<br> <br> It's gotten so bad we added a second way to do the C API:<br> <br> void qmp_drive_backup(DriveBackup *arg, Error **errp);<br> <br> Turns out<br> <br> DriveBackup arg = {<br> ... initialize the optionals you need ...<br> }<br> qmp_drive_backup(&arg, &err);<br> <br> is a lot easier on the eyes than passing 29 positional arguments.<br> <br></blockquote><div><br></div><div>So is writing the function arguments with indentation. Then I don't see much difference between a long list of arguments in a struct and that function. The main difference is that you make it easy to pass those arguments down. But often, you want to pass a subset, you don't want to pass the whole context as it may lead to bad design / bugs.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> This could be viewed as a work-around for C's lack of positional<br> parameters.<br> <br></blockquote><div><br></div><div>Or a badly designed QMP command.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Even more fun:<br> <br> void qmp_blockdev_add(BlockdevOptions *arg, Error **errp);<br> <br> BlockdevOptions is a tagged union.<br> <br> This could be viewed as a work-around for C's lack of function<br> overloading.<br> <br></blockquote><div><br></div><div>Or a badly designed QMP command ?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > It's easy for QEMU to generate a good static binding for C, because the<br> > version always matches. For a client, you wouldn't be able to write a<br> > similar idiomatic API in C, Rust, Python or Go, unfortunately.<br> <br> I disagree. You won't be able to write good bindings using just<br> positional parameters. Not even if you add restrictions on how we can<br> evolve QMP. And no, I do not consider the C bindings we create for QEMU<br> itself "good". They're the best we could do, and good enough.<br> <br></blockquote><div><br></div><div>Sure they could be better, they are still quite idiomatic for C.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> When you do bindings for another language, do bindings for that<br> language, not C bindings in that language.<br> <br></blockquote><div><br></div><div>Yes</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Regardless of bindings, the client as a whole should obey the rules of<br> the QMP game laid down in qmp-spec.txt and qapi-code-gen.txt. If these<br> rules have become counter-productive, then it's time to replace QMP<br> wholesale.<br> <br> Do not attempt to force a square peg into a round hole. If we must have<br> square pegs, design a square hole, and retire the round hole.<br> <br></blockquote><div><br></div><div>Hmm? I am trying to make the hole a bit more regular...<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > Iow, I am not trying to sell DBus, I would like to make it easier to bind<br> > QMP in general. (although I do believe that DBus is a better protocol than<br> > QMP for local IPC, yes. And gRPC is probably better for remoting)<br> ><br> >> I would rather make those problems solved at the server level, that<br> >> > doesn't require any change to QMP today, just a more careful<br> >> > consideration when making changes (and probably some tools to help<br> >> > enforce some stability).<br> >><br> >> Problem is, "more careful consideration when making changes" is not a<br> >> small thing. And other RPCs have evolved in a completely different<br> >> space (REST APIs for web services) that have chosen the same tradeoffs<br> >> as QMP, so why should we not learn from them?<br> >><br> >><br> > I don't buy that generalization. A very recent protocol in this space, that<br> > aims to be a good low-level RPC on Linux (for containers, cloud etc) is<br> > varlink. (In many ways, we could compare it to QMP, but it lacks some<br> > important features, like events)<br> ><br> > varlink does non-optional members and versioning the same way I propose<br> > here, for what I could tell. Although they use JSON, and have similar<br> > transport "benefits", this basic rule allow them to have very decent<br> > automated binding in various languages, without resorting to unorthodox<br> > solutions, ex:<br> > <a href="https://github.com/varlink/rust/blob/master/examples/example/src/main.rs" rel="noreferrer" target="_blank">https://github.com/varlink/rust/blob/master/examples/example/src/main.rs</a><br> <br> Paolo pointed out successful protocols that make tradeoffs similar to<br> QMP to support the idea that these tradeoffs can make sense and are<br> workable.<br> <br> Pointing out other, dissimilar protocols is not a convincing<br> counter-argument :)<br></blockquote><div><br></div><div>It's relevant. Did you study varlink a bit? It's so close to QMP, you will find it hard to point out real dissimilarities.<br></div></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
Hi On Tue, Sep 29, 2020 at 10:23 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 29/09/20 19:55, Marc-André Lureau wrote: > > My understanding of what you propose is: > > - ForeignConvert::with_foreign > > - FromForeign::from_foreign (with implied into_native) > > And: > > - ForeignConvert::as_foreign (with the BorrowedPointer/stash-like) > > - ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems > > wrongly designed in your proposal and unnecessary for now) > > Might well be, but how is it wrong? (I'd like to improve). > Why BorrowedMutPointer provides both *const P and *mut P ? The *const P conversion seems overlapping with BorrowedPointer. Anyway, the &mut T -> *mut P conversion seems fairly rare to me and error-prone. You usually give up ownership if you let the foreign function tweak the P. In any case, we don't need such conversion for QAPI, for now. > > I don't have your head, so I find it hard to remember & work with. It> > uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_. > > That just blows my mind, sorry :) > > Ahah I don't have your head either! The idea anyway is to reuse > prefixes that are common in Rust code: > > * with_: a constructor that uses something to build a type (think > Vec::with_capacity) and therefore takes ownership > ForeignConvert::with_foreign (const *P -> T) doesn't take ownership. The Rust reference for this kind of conversion is CStr::from_ptr. > * as_: a cheap conversion to something, it's cheap because it reuses the > lifetime (and therefore takes no ownership). Think Option::as_ref. > as_ shouldn't create any object, and is thus unsuitable for a general rs<->sys conversion function (any). * from_/to_: a copying and possibly expensive conversion (that you have > to write the code for). Because it's copying, it doesn't consume the > argument (for from_) or self (for to_). > > and that's what glib-rs uses (and CStr). > * into_: a conversion that consumes the receiver > > That's not used by glib afaik, but we should be able to introduce it for "mut *P -> T", it's not incompatible with FromPtrFull::from_full. In general, I like the fact that the conversion traits are associated to T, and not to P (which can remain a bare pointer, without much associated methods). It may well be over the top. > > > Then, I don't understand why ForeignConvert should hold both the "const > > *P -> T" and "&T -> const *P" conversions. Except the common types, > > what's the relation between the two? > > Maybe I'm wrong, but why would you need just one? > No I mean they could be on different traits. One could be implemented without the other. Or else I don't understand why the other conversion functions would not be in that trait too. > > Finally, I thought you introduced some differences with the stash > > design, but in fact I can see that ForeignConvert::Storage works just > > the way as ToPtr::Storage. So composition should be similar. Only your > > example code is more repetitive as it doesn't indirectly refer to the > > trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage). > > Yes, that's the main difference. I removed Storage because I didn't > want to force any trait on BorrowedPointer's second type argument. It > seemed like a generic concept to me. > To the cost of some duplication. I like the coupling between the traits better. If you need a similar tuple/struct elsewhere, it's easy to make your own. The Storage type can quickly become quite complicated with QAPI, I would rather avoid having to repeat it, it would create hideous compiler errors too. > The other difference is that Stash is a tuple while BorrowedPointer is a > struct and has methods to access it. Stash seems very ugly to use. > Yes I agree. Not sure why they made it a bare tuple, laziness perhaps :). -- Marc-André Lureau <div dir="ltr"><div dir="ltr">Hi<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 29, 2020 at 10:23 PM Paolo Bonzini <<a href="mailto:pbonzini@redhat.com">pbonzini@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 29/09/20 19:55, Marc-André Lureau wrote:<br> > My understanding of what you propose is:<br> > - ForeignConvert::with_foreign<br> > - FromForeign::from_foreign (with implied into_native)<br> > And:<br> > - ForeignConvert::as_foreign (with the BorrowedPointer/stash-like)<br> > - ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems<br> > wrongly designed in your proposal and unnecessary for now)<br> <br> Might well be, but how is it wrong? (I'd like to improve).<br></blockquote><div><br></div>Why BorrowedMutPointer provides both *const P and *mut P ? The *const P conversion seems overlapping with BorrowedPointer.</div><div class="gmail_quote"><br></div><div class="gmail_quote">Anyway, the &mut T -> *mut P conversion seems fairly rare to me and error-prone. You usually give up ownership if you let the foreign function tweak the P.<br></div><div class="gmail_quote"><div><br></div><div>In any case, we don't need such conversion for QAPI, for now.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > I don't have your head, so I find it hard to remember & work with. It> uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_.<br> > That just blows my mind, sorry :)<br> <br> Ahah I don't have your head either! The idea anyway is to reuse<br> prefixes that are common in Rust code:<br> <br> * with_: a constructor that uses something to build a type (think<br> Vec::with_capacity) and therefore takes ownership<br></blockquote><div><br></div><div><br></div><div>ForeignConvert::with_foreign (const *P -> T) doesn't take ownership.</div><div><br></div><div>The Rust reference for this kind of conversion is CStr::from_ptr.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> * as_: a cheap conversion to something, it's cheap because it reuses the<br> lifetime (and therefore takes no ownership). Think Option::as_ref.<br></blockquote><div><br></div><div>as_ shouldn't create any object, and is thus unsuitable for a general rs<->sys conversion function (any). <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> * from_/to_: a copying and possibly expensive conversion (that you have<br> to write the code for). Because it's copying, it doesn't consume the<br> argument (for from_) or self (for to_).<br> <br></blockquote><div><br></div><div>and that's what glib-rs uses (and CStr).</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> * into_: a conversion that consumes the receiver<br> <br></blockquote><div><br></div><div>That's not used by glib afaik, but we should be able to introduce it for "mut *P -> T", it's not incompatible with FromPtrFull::from_full.</div><div><br></div><div>In general, I like the fact that the conversion traits are associated to T, and not to P (which can remain a bare pointer, without much associated methods).<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> It may well be over the top.<br> <br> > Then, I don't understand why ForeignConvert should hold both the "const<br> > *P -> T" and "&T -> const *P" conversions. Except the common types,<br> > what's the relation between the two?<br> <br> Maybe I'm wrong, but why would you need just one?<br></blockquote><div><br></div><div>No I mean they could be on different traits. One could be implemented without the other. Or else I don't understand why the other conversion functions would not be in that trait too.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > Finally, I thought you introduced some differences with the stash<br> > design, but in fact I can see that ForeignConvert::Storage works just<br> > the way as ToPtr::Storage. So composition should be similar. Only your<br> > example code is more repetitive as it doesn't indirectly refer to the<br> > trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage).<br> <br> Yes, that's the main difference. I removed Storage because I didn't<br> want to force any trait on BorrowedPointer's second type argument. It<br> seemed like a generic concept to me.<br></blockquote><div><br></div><div>To the cost of some duplication. I like the coupling between the traits better. If you need a similar tuple/struct elsewhere, it's easy to make your own.</div><div><br></div><div>The Storage type can quickly become quite complicated with QAPI, I would rather avoid having to repeat it, it would create hideous compiler errors too.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> The other difference is that Stash is a tuple while BorrowedPointer is a<br> struct and has methods to access it. Stash seems very ugly to use.<br></blockquote><div><br></div><div>Yes I agree. Not sure why they made it a bare tuple, laziness perhaps :).</div><br></div><br>-- <br><div dir="ltr" class="gmail_signature">Marc-André Lureau<br></div></div>
On 30/09/20 09:51, Marc-André Lureau wrote: > Hi > > On Wed, Sep 30, 2020 at 11:34 AM Markus Armbruster <armbru@redhat.com > <mailto:armbru@redhat.com>> wrote: > > Marc-André Lureau <marcandre.lureau@gmail.com > <mailto:marcandre.lureau@gmail.com>> writes: > > > Hi > > > > On Tue, Sep 29, 2020 at 3:01 PM Paolo Bonzini <pbonzini@redhat.com > <mailto:pbonzini@redhat.com>> wrote: > [...] > >> Marc-André, we are totally in agreement about that! The problem > is that > >> you have already decided what the solution looks like, and that's > what > >> I'm not sure about because your solution also implies completely > >> revisiting the schema. > >> > > > > Did I? Which schema change are you (or I) implying? Versioning the > > interface? It's necessary at the client level, unless everything is > > dynamic, after introspection, which makes automated static bindings > > impractical. > > I disagree with "necessary". > > A client can use a specific version of QMP, and still talk to a server > with a different version, because we designed that capability into QMP. > > "A client can use a specific version of QMP" == versioning on the > client side No: "a client can use code generated for a specific version of QMP" does not imply versioning on the client side. It does imply that, before using *certain* features, the client must verify that they are implemented in the server. > 1. Opaque dictionaries are far from the only way to do keyword arguments > in a language that lacks them. > > Oh one can always be creative. The point is trying to stay idiomatic in > the target language. And builders in Rust, or keyword arguments in Python, are perfectly idiomatic. > Or a badly designed QMP command. > Or a badly designed QMP command ? Great. Do you have ideas around how to design good QMP commands? It doesn't have to be patches for code, just documentation. > > varlink does non-optional members and versioning the same way I propose > > here, for what I could tell. Although they use JSON, and have similar > > transport "benefits", this basic rule allow them to have very decent > > automated binding in various languages, without resorting to > unorthodox > > solutions, ex: > > > https://github.com/varlink/rust/blob/master/examples/example/src/main.rs > > Paolo pointed out successful protocols that make tradeoffs similar to > QMP to support the idea that these tradeoffs can make sense and are > workable. > > Pointing out other, dissimilar protocols is not a convincing > counter-argument :) > > It's relevant. Did you study varlink a bit? It's so close to QMP, you > will find it hard to point out real dissimilarities. I played a bit with varlink, and it seems to me that varlink does actually support QMP-/OpenAPI-like extensibility. Missing nullable fields are treated as null, and on the client side it generally does not give an error if it receives unknown fields (just like QMP). [1] Also just like QMP there are limits to the server's liberality, for example sending extra fields to the server is an error. In fact I could have skipped the experiments and read the Varlink documentation: :) Varlink interfaces do not have a version number, they only have a feature set described in detail by the interface definition, which is part of the wire protocol. If your interface has users, you should not break the interface, only extend it, never remove or incompatibly change things which might be already in use. [...] Varlink does not use positional parameters or fixed-size objects in its interface definition or on the wire, all parameters are identified by their name and can be extended later. Extending existing structures should be done via optional fields (nullable type, maybe). The result of the methods, when passed parameters without the optional fields should be the same as in older versions. Method parameters can be extended the same way. The expected behavior for omitted fields/parameters should be documented. Removing fields, types, errors or methods are not backward compatible and should be avoided. which is pretty much what QMP does. So even though some users of varlink might have chosen to rein themselves in on the extensibility allowed by Varlink, that's a self-imposed limitation. It may be due to the desire to interoperate with varlink language bindings that use positional parameters, but that's again a limitation of the bindings and not the protocol. So I don't see varlink as a reason to change the existing practice for QMP, more as a second example of someone else doing the same as QMP. Again, I'm not saying that DBus's choice with respect to versioning and backwards/forwards-compatibility is _wrong_. I do not like Protobuf's numbered fields for that matter either, but it's not wrong either. I am saying that the choices in QMP are one of many different tradeoffs, and that there's no point in saying that they get in the way of writing language bindings or even just in the way of writing good/idiomatic language bindings, because they don't. Paolo [1] Try this: $ git clone git://github.com/bonzini/python $ git checkout nullable-ping $ python3 -m varlink.tests.test_orgexamplemore --varlink="unix:/tmp/test" & And then: $ varlink info unix:/tmp/test org.example.more ... method Ping(ping: ?string) -> (pong: ?string) ... $ python -m varlink.cli call unix:/tmp/test/org.example.more.Ping '{}' { "pang": "xxx", "pong": null } You can see that the argument was implicitly treated as null, and there were no complaints about the extra returned field. I didn't try returning extra fields with the Rust bindings, but I did try optional arguments and they work fine. A "recent" client with optional argument can talk to an "old" server with non-optional argument, and it will only fail if the client actually takes advantage of the newer interface. As long as the argument is there, everything works just fine.
On 30/09/20 11:15, Marc-André Lureau wrote: > Why BorrowedMutPointer provides both *const P and *mut P ? The *const P > conversion seems overlapping with BorrowedPointer. "&mut T" implements Borrow so it seemed obvious to have as_ptr in BorrowedMutPointer too. Though I certainly should implement Borrow in BorrowedMutPointer. > > I don't have your head, so I find it hard to remember & work with. > It> uses all possible prefixes: with_, from_, as_, as_mut, to_, and > into_. > > That just blows my mind, sorry :) > > Ahah I don't have your head either! The idea anyway is to reuse > prefixes that are common in Rust code: > > * with_: a constructor that uses something to build a type (think > Vec::with_capacity) and therefore takes ownership > > ForeignConvert::with_foreign (const *P -> T) doesn't take ownership. > > The Rust reference for this kind of conversion is CStr::from_ptr. Ok, I'll take a look. > * as_: a cheap conversion to something, it's cheap because it reuses the > lifetime (and therefore takes no ownership). Think Option::as_ref. > > as_ shouldn't create any object, and is thus unsuitable for a general > rs<->sys conversion function (any). as_foreign function does not create anything, it reuses the storage to provide a pointer. It seems similar to as_slice for example. > * from_/to_: a copying and possibly expensive conversion (that you have > to write the code for). Because it's copying, it doesn't consume the > argument (for from_) or self (for to_). > > and that's what glib-rs uses (and CStr). Sort of, I found the none/full suffixes not really idiomatic for Rust. > * into_: a conversion that consumes the receiver > > That's not used by glib afaik, but we should be able to introduce it for > "mut *P -> T", it's not incompatible with FromPtrFull::from_full. Right. It's just a different way to write the same thing. Usually it is a bit more concise because it allows more type inference. > > Then, I don't understand why ForeignConvert should hold both the > > "const *P -> T" and "&T -> const *P" conversions. Except the > > common types, what's the relation between the two? > > Maybe I'm wrong, but why would you need just one? > > No I mean they could be on different traits. One could be implemented > without the other. Or else I don't understand why the other conversion > functions would not be in that trait too. The other conversion functions require taking ownership, and I was not sure if it would always be possible to do so. For no-ownership-taken conversions, however, it seemed to me that you'd rarely be unable to implement one of the two directions. I might be wrong. In general though I agree that the changes are mostly cosmetic. Paolo
diff --git a/Cargo.toml b/Cargo.toml new file mode 100644 index 0000000000..e69b04200f --- /dev/null +++ b/Cargo.toml @@ -0,0 +1,2 @@ +[workspace] +members = ["qga"] diff --git a/meson.build b/meson.build index 5421eca66a..27a7064a47 100644 --- a/meson.build +++ b/meson.build @@ -637,7 +637,11 @@ qapi_gen_depends = [ meson.source_root() / 'scripts/qapi/__init__.py', meson.source_root() / 'scripts/qapi/visit.py', meson.source_root() / 'scripts/qapi/common.py', meson.source_root() / 'scripts/qapi/doc.py', - meson.source_root() / 'scripts/qapi-gen.py' + meson.source_root() / 'scripts/qapi/rs.py', + meson.source_root() / 'scripts/qapi/rs_sys.py', + meson.source_root() / 'scripts/qapi/rs_types.py', + meson.source_root() / 'scripts/qapi/rs_dbus.py', + meson.source_root() / 'scripts/qapi-gen.py', ] tracetool = [ diff --git a/qga/Cargo.toml b/qga/Cargo.toml new file mode 100644 index 0000000000..b0e6fe62ce --- /dev/null +++ b/qga/Cargo.toml @@ -0,0 +1,22 @@ +[package] +name = "qga" +version = "0.1.0" +edition = "2018" + +[features] +default = ["dbus"] +dbus = ["serde", "serde_repr", "zbus", "zvariant", "zvariant_derive"] + +[dependencies] +libc = "^0.2.76" +serde = { version = "^1.0.115", optional = true } +serde_repr = { version = "0.1.6", optional = true } +zbus = { git = "https://gitlab.freedesktop.org/zeenix/zbus", optional = true } +zvariant = { git = "https://gitlab.freedesktop.org/zeenix/zbus", optional = true } +zvariant_derive = { git = "https://gitlab.freedesktop.org/zeenix/zbus", optional = true } +hostname = "0.3.1" + +[lib] +name = "qga" +path = "lib.rs" +crate-type = ["staticlib"] diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 1a62a3a70d..244bf04acb 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -2160,36 +2160,6 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp) return NULL; } -int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp) -{ - int64_t processed; - Error *local_err = NULL; - - processed = 0; - while (vcpus != NULL) { - char *path = g_strdup_printf("/sys/devices/system/cpu/cpu%" PRId64 "/", - vcpus->value->logical_id); - - transfer_vcpu(vcpus->value, false, path, &local_err); - g_free(path); - if (local_err != NULL) { - break; - } - ++processed; - vcpus = vcpus->next; - } - - if (local_err != NULL) { - if (processed == 0) { - error_propagate(errp, local_err); - } else { - error_free(local_err); - } - } - - return processed; -} - void qmp_guest_set_user_password(const char *username, const char *password, bool crypted, diff --git a/qga/commands.c b/qga/commands.c index d3fec807c1..2a0db4623e 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -512,26 +512,6 @@ int ga_parse_whence(GuestFileWhence *whence, Error **errp) return -1; } -GuestHostName *qmp_guest_get_host_name(Error **errp) -{ - GuestHostName *result = NULL; - g_autofree char *hostname = qemu_get_host_name(errp); - - /* - * We want to avoid using g_get_host_name() because that - * caches the result and we wouldn't reflect changes in the - * host name. - */ - - if (!hostname) { - hostname = g_strdup("localhost"); - } - - result = g_new0(GuestHostName, 1); - result->host_name = g_steal_pointer(&hostname); - return result; -} - GuestTimezone *qmp_guest_get_timezone(Error **errp) { GuestTimezone *info = NULL; diff --git a/qga/error.rs b/qga/error.rs new file mode 100644 index 0000000000..edd2eb9fb2 --- /dev/null +++ b/qga/error.rs @@ -0,0 +1,90 @@ +use std::{self, ffi, fmt, ptr, io}; + +use crate::qemu; +use crate::qemu_sys; +use crate::translate::*; + +#[derive(Debug)] +pub enum Error { + FailedAt(String, &'static str, u32), + Io(io::Error), +} + +pub type Result<T> = std::result::Result<T, Error>; + +impl Error { + fn message(&self) -> String { + use Error::*; + match self { + FailedAt(msg, _, _) => msg.into(), + Io(io) => format!("IO error: {}", io), + } + } + + fn location(&self) -> Option<(&'static str, u32)> { + use Error::*; + match self { + FailedAt(_, file, line) => Some((file, *line)), + Io(_) => None, + } + } +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use Error::*; + match self { + FailedAt(msg, file, line) => write!(f, "Failed: {} ({}:{})", msg, file, line), + Io(io) => write!(f, "IO error: {}", io), + } + } +} + +impl From<io::Error> for Error { + fn from(val: io::Error) -> Self { + Error::Io(val) + } +} + +impl QemuPtrDefault for Error { + type QemuType = *mut qemu_sys::Error; +} + +impl<'a> ToQemuPtr<'a, *mut qemu_sys::Error> for Error { + type Storage = qemu::Error; + + fn to_qemu_none(&'a self) -> Stash<'a, *mut qemu_sys::Error, Self> { + let err = self.to_qemu_full(); + + Stash(err, unsafe { qemu::Error::from_raw(err) }) + } + + fn to_qemu_full(&self) -> *mut qemu_sys::Error { + let cmsg = + ffi::CString::new(self.message()).expect("ToQemuPtr<Error>: unexpected '\0' character"); + let mut csrc = ffi::CString::new("").unwrap(); + let (src, line) = self.location().map_or((ptr::null(), 0 as i32), |loc| { + csrc = ffi::CString::new(loc.0).expect("ToQemuPtr<Error>:: unexpected '\0' character"); + (csrc.as_ptr() as *const libc::c_char, loc.1 as i32) + }); + let func = ptr::null(); + + let mut err: *mut qemu_sys::Error = ptr::null_mut(); + unsafe { + qemu_sys::error_setg_internal( + &mut err as *mut *mut _, + src, + line, + func, + cmsg.as_ptr() as *const libc::c_char, + ); + err + } + } +} + +macro_rules! err { + ($err:expr) => { + Err(crate::error::Error::FailedAt($err.into(), file!(), line!())) + }; +} diff --git a/qga/lib.rs b/qga/lib.rs new file mode 100644 index 0000000000..6e927fd03b --- /dev/null +++ b/qga/lib.rs @@ -0,0 +1,11 @@ +#[macro_use] +mod error; +mod qapi; +mod qapi_sys; +mod qemu; +mod qemu_sys; +mod translate; +mod qmp; + +#[cfg(feature = "dbus")] +mod qapi_dbus; diff --git a/qga/main.c b/qga/main.c index 3febf3b0fd..89eec0d425 100644 --- a/qga/main.c +++ b/qga/main.c @@ -17,6 +17,7 @@ #ifndef _WIN32 #include <syslog.h> #include <sys/wait.h> +#include <glib-unix.h> #endif #include "qemu-common.h" #include "qapi/qmp/json-parser.h" @@ -73,6 +74,13 @@ typedef struct GAPersistentState { typedef struct GAConfig GAConfig; +typedef struct _QemuDBus QemuDBus; + +extern QemuDBus *qemu_dbus_new(void); +extern void qemu_dbus_free(QemuDBus *dbus); +extern int qemu_dbus_fd(QemuDBus *dbus); +extern void qemu_dbus_next(QemuDBus *dbus); + struct GAState { JSONMessageParser parser; GMainLoop *main_loop; @@ -102,6 +110,7 @@ struct GAState { GAConfig *config; int socket_activation; bool force_exit; + QemuDBus *dbus; }; struct GAState *ga_state; @@ -1261,6 +1270,13 @@ static bool check_is_frozen(GAState *s) return false; } +static gboolean dbus_cb(gint fd, GIOCondition condition, gpointer data) +{ + GAState *s = data; + qemu_dbus_next(s->dbus); + return G_SOURCE_CONTINUE; +} + static GAState *initialize_agent(GAConfig *config, int socket_activation) { GAState *s = g_new0(GAState, 1); @@ -1354,6 +1370,14 @@ static GAState *initialize_agent(GAConfig *config, int socket_activation) s->main_loop = g_main_loop_new(NULL, false); + { + s->dbus = qemu_dbus_new(); + int fd = qemu_dbus_fd(s->dbus); + GSource *source = g_unix_fd_source_new(fd, G_IO_IN); + g_source_set_callback(source, (GSourceFunc) dbus_cb, s, NULL); + g_source_attach(source, NULL); + } + s->config = config; s->socket_activation = socket_activation; diff --git a/qga/meson.build b/qga/meson.build index e5c5778a3e..ec8b7e7f39 100644 --- a/qga/meson.build +++ b/qga/meson.build @@ -46,9 +46,49 @@ qga_ss.add(when: 'CONFIG_WIN32', if_true: files( qga_ss = qga_ss.apply(config_host, strict: false) +find_program('cargo', required: true) +find_program('rustfmt', required: true) +cargo_sh = find_program('../scripts/cargo.sh', required: true) + +run_target('cargo-clippy', + command: [cargo_sh, 'clippy']) + +qga_qapi_rs_outputs = [ + 'qga-qapi-sys-types.rs', + 'qga-qapi-types.rs', + 'qga-qapi-dbus.rs', +] + +qga_qapi_rs_files = custom_target('QGA QAPI Rust files', + output: qga_qapi_rs_outputs, + input: 'qapi-schema.json', + command: [ qapi_gen, '-r', '-o', 'qga', '-p', 'qga-', '@INPUT0@' ], + depend_files: qapi_gen_depends) + +rs_files = files( + 'Cargo.toml', + 'error.rs', + 'lib.rs', + 'qapi.rs', + 'qapi_dbus.rs', + 'qapi_sys.rs', + 'qemu.rs', + 'qemu_sys.rs', + 'qmp.rs', + 'translate.rs', +) + +buildtype = get_option('buildtype') +cargo_build = custom_target('cargo-build', + input: [qga_qapi_rs_files, rs_files], + output: ['cargo-build.stamp'], + console: true, + command: [cargo_sh, 'build', buildtype, meson.current_build_dir(), meson.source_root(), meson.build_root()]) +qga_rs = declare_dependency(link_args: ['-lrt', '-ldl', 'rs-target/@0@/libqga.a'.format(buildtype)], sources: cargo_build) + qga = executable('qemu-ga', qga_ss.sources(), link_args: config_host['LIBS_QGA'].split(), - dependencies: [qemuutil, libudev], + dependencies: [qemuutil, libudev, qga_rs], install: true) all_qga = [qga] diff --git a/qga/qapi.rs b/qga/qapi.rs new file mode 100644 index 0000000000..5065bb6930 --- /dev/null +++ b/qga/qapi.rs @@ -0,0 +1,121 @@ +#![allow(dead_code)] +use std::convert::{TryFrom, TryInto}; +use std::{ptr, str}; + +#[cfg(feature = "dbus")] +use zvariant::OwnedValue; +#[cfg(feature = "dbus")] +use serde::{Deserialize, Serialize}; +#[cfg(feature = "dbus")] +use zvariant_derive::{Type, TypeDict, SerializeDict, DeserializeDict}; + +use crate::translate::*; + +use crate::translate; +use crate::qapi_sys; +use crate::qemu_sys; + +include!(concat!(env!("MESON_BUILD_ROOT"), "/qga/qga-qapi-types.rs")); + +impl<'a> ToQemuPtr<'a, *mut qapi_sys::GuestFileWhence> for GuestFileWhence { + type Storage = Box<qapi_sys::GuestFileWhence>; + + #[inline] + fn to_qemu_none(&'a self) -> Stash<'a, *mut qapi_sys::GuestFileWhence, GuestFileWhence> { + let mut w = Box::new(self.into()); + + Stash(&mut *w, w) + } +} + +impl From<&GuestFileWhence> for qapi_sys::GuestFileWhence { + fn from(w: &GuestFileWhence) -> Self { + match *w { + GuestFileWhence::Name(name) => Self { + ty: QType::Qstring, + u: qapi_sys::GuestFileWhenceUnion { name }, + }, + GuestFileWhence::Value(value) => Self { + ty: QType::Qnum, + u: qapi_sys::GuestFileWhenceUnion { value }, + }, + } + } +} + +#[cfg(feature = "dbus")] +impl From<GuestFileWhence> for OwnedValue { + fn from(_w: GuestFileWhence) -> Self { + unimplemented!() + } +} + +#[cfg(feature = "dbus")] +impl TryFrom<OwnedValue> for GuestFileWhence { + type Error = &'static str; + + fn try_from(value: OwnedValue) -> Result<Self, Self::Error> { + if let Ok(val) = (&value).try_into() { + return Ok(Self::Name(match val { + "set" => QGASeek::Set, + "cur" => QGASeek::Cur, + "end" => QGASeek::End, + _ => return Err("Invalid seek value"), + })); + } + if let Ok(val) = value.try_into() { + return Ok(Self::Value(val)); + }; + Err("Invalid whence") + } +} + +macro_rules! vec_to_qemu_ptr { + ($rs:ident, $sys:ident) => { + #[allow(non_camel_case_types)] + pub struct $sys(*mut qapi_sys::$sys); + + impl Drop for $sys { + fn drop(&mut self) { + let mut list = self.0; + unsafe { + while !list.is_null() { + let next = (*list).next; + Box::from_raw(list); + list = next; + } + } + } + } + + impl<'a> ToQemuPtr<'a, *mut qapi_sys::$sys> for Vec<$rs> { + type Storage = ( + Option<$sys>, + Vec<Stash<'a, <$rs as QemuPtrDefault>::QemuType, $rs>>, + ); + + #[inline] + fn to_qemu_none(&self) -> Stash<*mut qapi_sys::$sys, Self> { + let stash_vec: Vec<_> = self.iter().rev().map(ToQemuPtr::to_qemu_none).collect(); + let mut list: *mut qapi_sys::$sys = ptr::null_mut(); + for stash in &stash_vec { + let b = Box::new(qapi_sys::$sys { + next: list, + value: Ptr::to(stash.0), + }); + list = Box::into_raw(b); + } + Stash(list, (Some($sys(list)), stash_vec)) + } + } + }; +} + +// TODO: could probably be templated instead +vec_to_qemu_ptr!(String, strList); +vec_to_qemu_ptr!(GuestAgentCommandInfo, GuestAgentCommandInfoList); +vec_to_qemu_ptr!(GuestFilesystemTrimResult, GuestFilesystemTrimResultList); +vec_to_qemu_ptr!(GuestIpAddress, GuestIpAddressList); +vec_to_qemu_ptr!(GuestDiskAddress, GuestDiskAddressList); +vec_to_qemu_ptr!(GuestLogicalProcessor, GuestLogicalProcessorList); +vec_to_qemu_ptr!(GuestMemoryBlock, GuestMemoryBlockList); diff --git a/qga/qapi_dbus.rs b/qga/qapi_dbus.rs new file mode 100644 index 0000000000..2713c6e1a4 --- /dev/null +++ b/qga/qapi_dbus.rs @@ -0,0 +1,99 @@ +use std::convert::TryInto; +use std::error::Error; +use std::ffi::CString; +use std::os::unix::io::{AsRawFd, RawFd}; +use std::ptr; + +use zbus::fdo; +use zbus::{dbus_interface, Connection, DBusError, ObjectServer}; + +use crate::qapi; +use crate::qapi_sys; +use crate::qemu; +use crate::qemu_sys; +use crate::translate::*; + +include!(concat!(env!("MESON_BUILD_ROOT"), "/qga/qga-qapi-dbus.rs")); + +#[derive(Debug, DBusError)] +#[dbus_error(prefix = "org.qemu.QapiError")] +pub enum QapiError { + /// ZBus error + ZBus(zbus::Error), + /// QMP error + Failed(String), +} + +impl FromQemuPtrFull<*mut qemu_sys::Error> for QapiError { + unsafe fn from_qemu_full(ptr: *mut qemu_sys::Error) -> Self { + QapiError::Failed(qemu::Error::from_raw(ptr).pretty().to_string()) + } +} + +type Result<T> = std::result::Result<T, QapiError>; + +#[derive(Debug)] +pub struct QemuDBus { + pub connection: Connection, + pub server: ObjectServer<'static>, +} + +impl QemuDBus { + fn open(name: &str) -> std::result::Result<Self, Box<dyn Error>> { + let connection = Connection::new_session()?; + + fdo::DBusProxy::new(&connection)? + .request_name(name, fdo::RequestNameFlags::ReplaceExisting.into())?; + + let server = ObjectServer::new(&connection); + Ok(Self { connection, server }) + } +} + +#[no_mangle] +extern "C" fn qemu_dbus_new() -> *mut QemuDBus { + let mut dbus = match QemuDBus::open(&"org.qemu.qga") { + Ok(dbus) => dbus, + Err(e) => { + eprintln!("{}", e); + return std::ptr::null_mut(); + } + }; + dbus.server + .at(&"/org/qemu/qga".try_into().unwrap(), QgaQapi) + .unwrap(); + + Box::into_raw(Box::new(dbus)) +} + +#[no_mangle] +extern "C" fn qemu_dbus_free(dbus: *mut QemuDBus) { + let dbus = unsafe { + assert!(!dbus.is_null()); + Box::from_raw(dbus) + }; + // let's be explicit: + drop(dbus) +} + +#[no_mangle] +extern "C" fn qemu_dbus_fd(dbus: *mut QemuDBus) -> RawFd { + let dbus = unsafe { + assert!(!dbus.is_null()); + &mut *dbus + }; + + dbus.connection.as_raw_fd() +} + +#[no_mangle] +extern "C" fn qemu_dbus_next(dbus: *mut QemuDBus) { + let dbus = unsafe { + assert!(!dbus.is_null()); + &mut *dbus + }; + + if let Err(err) = dbus.server.try_handle_next() { + eprintln!("{}", err); + } +} diff --git a/qga/qapi_sys.rs b/qga/qapi_sys.rs new file mode 100644 index 0000000000..06fc49b826 --- /dev/null +++ b/qga/qapi_sys.rs @@ -0,0 +1,5 @@ +#![allow(dead_code)] +include!(concat!( + env!("MESON_BUILD_ROOT"), + "/qga/qga-qapi-sys-types.rs" +)); diff --git a/qga/qemu.rs b/qga/qemu.rs new file mode 100644 index 0000000000..5aad9a2b55 --- /dev/null +++ b/qga/qemu.rs @@ -0,0 +1,30 @@ +use std::ffi::CStr; +/// or do something full-fledged like glib-rs boxed MM... +use std::ptr; +use std::str; + +use crate::qemu_sys; + +pub struct Error(ptr::NonNull<qemu_sys::Error>); + +impl Error { + pub unsafe fn from_raw(ptr: *mut qemu_sys::Error) -> Self { + assert!(!ptr.is_null()); + Self(ptr::NonNull::new_unchecked(ptr)) + } + + pub fn pretty(&self) -> &str { + unsafe { + let pretty = qemu_sys::error_get_pretty(self.0.as_ptr()); + let bytes = CStr::from_ptr(pretty).to_bytes(); + str::from_utf8(bytes) + .unwrap_or_else(|err| str::from_utf8(&bytes[..err.valid_up_to()]).unwrap()) + } + } +} + +impl Drop for Error { + fn drop(&mut self) { + unsafe { qemu_sys::error_free(self.0.as_ptr()) } + } +} diff --git a/qga/qemu_sys.rs b/qga/qemu_sys.rs new file mode 100644 index 0000000000..04fc0d9f9d --- /dev/null +++ b/qga/qemu_sys.rs @@ -0,0 +1,50 @@ +use libc::{c_char, c_void, size_t}; + +extern "C" { + pub fn g_malloc0(n_bytes: size_t) -> *mut c_void; + pub fn g_free(ptr: *mut c_void); + pub fn g_strndup(str: *const c_char, n: size_t) -> *mut c_char; +} + +#[repr(C)] +pub struct QObject(c_void); + +impl ::std::fmt::Debug for QObject { + fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result { + f.debug_struct(&format!("QObject @ {:?}", self as *const _)) + .finish() + } +} + +#[repr(C)] +pub struct QNull(c_void); + +impl ::std::fmt::Debug for QNull { + fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result { + f.debug_struct(&format!("QNull @ {:?}", self as *const _)) + .finish() + } +} + +#[repr(C)] +pub struct Error(c_void); + +impl ::std::fmt::Debug for Error { + fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result { + f.debug_struct(&format!("Error @ {:?}", self as *const _)) + .finish() + } +} + +extern "C" { + pub fn error_setg_internal( + errp: *mut *mut Error, + src: *const c_char, + line: i32, + func: *const c_char, + fmt: *const c_char, + ... + ); + pub fn error_get_pretty(err: *const Error) -> *const c_char; + pub fn error_free(err: *mut Error); +} diff --git a/qga/qmp.rs b/qga/qmp.rs new file mode 100644 index 0000000000..0224e7e4fb --- /dev/null +++ b/qga/qmp.rs @@ -0,0 +1,67 @@ +use std::ptr; + +use crate::error::Result; +use crate::qapi; +use crate::qapi_sys; +use crate::qemu_sys; +use crate::translate::*; + +macro_rules! qmp { + // the basic return value variant + ($e:expr, $errp:ident, $errval:expr) => {{ + assert!(!$errp.is_null()); + unsafe { + *$errp = ptr::null_mut(); + } + + match $e { + Ok(val) => val, + Err(err) => unsafe { + *$errp = err.to_qemu_full(); + $errval + }, + } + }}; + // the ptr return value variant + ($e:expr, $errp:ident) => {{ + assert!(!$errp.is_null()); + unsafe { + *$errp = ptr::null_mut(); + } + + match $e { + Ok(val) => val.to_qemu_full(), + Err(err) => unsafe { + *$errp = err.to_qemu_full(); + ptr::null_mut() + }, + } + }}; +} + +fn guest_host_name() -> Result<qapi::GuestHostName> { + Ok(qapi::GuestHostName { + host_name: hostname::get()?.into_string().or(err!("Invalid hostname"))?, + }) +} + +#[no_mangle] +extern "C" fn qmp_guest_get_host_name( + errp: *mut *mut qemu_sys::Error, +) -> *mut qapi_sys::GuestHostName { + qmp!(guest_host_name(), errp) +} + +fn guest_set_vcpus(vcpus: Vec<qapi::GuestLogicalProcessor>) -> Result<i64> { + dbg!(vcpus); + err!("unimplemented") +} + +#[no_mangle] +extern "C" fn qmp_guest_set_vcpus( + vcpus: *const qapi_sys::GuestLogicalProcessorList, + errp: *mut *mut qemu_sys::Error, +) -> libc::c_longlong { + let vcpus = unsafe { from_qemu_none(vcpus) }; + qmp!(guest_set_vcpus(vcpus), errp, -1) +} diff --git a/qga/translate.rs b/qga/translate.rs new file mode 100644 index 0000000000..715951f2ba --- /dev/null +++ b/qga/translate.rs @@ -0,0 +1,173 @@ +// largely adapted from glib-rs +// we don't depend on glib-rs as this brings a lot more code that we may not need +// and also because there are issues with the conversion traits for our sys::*mut. +use libc::{c_char, size_t}; +use std::ffi::{CStr, CString}; +use std::ptr; + +use crate::qemu_sys; + +pub trait Ptr: Copy + 'static { + fn is_null(&self) -> bool; + fn from<X>(ptr: *mut X) -> Self; + fn to<X>(self) -> *mut X; +} + +impl<T: 'static> Ptr for *const T { + #[inline] + fn is_null(&self) -> bool { + (*self).is_null() + } + + #[inline] + fn from<X>(ptr: *mut X) -> *const T { + ptr as *const T + } + + #[inline] + fn to<X>(self) -> *mut X { + self as *mut X + } +} + +impl<T: 'static> Ptr for *mut T { + #[inline] + fn is_null(&self) -> bool { + (*self).is_null() + } + + #[inline] + fn from<X>(ptr: *mut X) -> *mut T { + ptr as *mut T + } + + #[inline] + fn to<X>(self) -> *mut X { + self as *mut X + } +} + +/// Provides the default pointer type to be used in some container conversions. +/// +/// It's `*mut c_char` for `String`, `*mut GtkButton` for `gtk::Button`, etc. +pub trait QemuPtrDefault { + type QemuType: Ptr; +} + +impl QemuPtrDefault for String { + type QemuType = *mut c_char; +} + +pub struct Stash<'a, P: Copy, T: ?Sized + ToQemuPtr<'a, P>>( + pub P, + pub <T as ToQemuPtr<'a, P>>::Storage, +); + +/// Translate to a pointer. +pub trait ToQemuPtr<'a, P: Copy> { + type Storage; + + /// Transfer: none. + /// + /// The pointer in the `Stash` is only valid for the lifetime of the `Stash`. + fn to_qemu_none(&'a self) -> Stash<'a, P, Self>; + + /// Transfer: full. + /// + /// We transfer the ownership to the foreign library. + fn to_qemu_full(&self) -> P { + unimplemented!(); + } +} + +impl<'a, P: Ptr, T: ToQemuPtr<'a, P>> ToQemuPtr<'a, P> for Option<T> { + type Storage = Option<<T as ToQemuPtr<'a, P>>::Storage>; + + #[inline] + fn to_qemu_none(&'a self) -> Stash<'a, P, Option<T>> { + self.as_ref() + .map_or(Stash(Ptr::from::<()>(ptr::null_mut()), None), |s| { + let s = s.to_qemu_none(); + Stash(s.0, Some(s.1)) + }) + } + + #[inline] + fn to_qemu_full(&self) -> P { + self.as_ref() + .map_or(Ptr::from::<()>(ptr::null_mut()), ToQemuPtr::to_qemu_full) + } +} + +impl<'a> ToQemuPtr<'a, *mut c_char> for String { + type Storage = CString; + + #[inline] + fn to_qemu_none(&self) -> Stash<'a, *mut c_char, String> { + let tmp = CString::new(&self[..]) + .expect("String::ToQemuPtr<*mut c_char>: unexpected '\0' character"); + Stash(tmp.as_ptr() as *mut c_char, tmp) + } + + #[inline] + fn to_qemu_full(&self) -> *mut c_char { + unsafe { qemu_sys::g_strndup(self.as_ptr() as *const c_char, self.len() as size_t) } + } +} + +pub trait FromQemuPtrNone<P: Ptr>: Sized { + unsafe fn from_qemu_none(ptr: P) -> Self; +} + +pub trait FromQemuPtrFull<P: Ptr>: Sized { + unsafe fn from_qemu_full(ptr: P) -> Self; +} + +#[inline] +pub unsafe fn from_qemu_none<P: Ptr, T: FromQemuPtrNone<P>>(ptr: P) -> T { + FromQemuPtrNone::from_qemu_none(ptr) +} + +#[inline] +pub unsafe fn from_qemu_full<P: Ptr, T: FromQemuPtrFull<P>>(ptr: P) -> T { + FromQemuPtrFull::from_qemu_full(ptr) +} + +impl<P: Ptr, T: FromQemuPtrNone<P>> FromQemuPtrNone<P> for Option<T> { + #[inline] + unsafe fn from_qemu_none(ptr: P) -> Option<T> { + if ptr.is_null() { + None + } else { + Some(from_qemu_none(ptr)) + } + } +} + +impl<P: Ptr, T: FromQemuPtrFull<P>> FromQemuPtrFull<P> for Option<T> { + #[inline] + unsafe fn from_qemu_full(ptr: P) -> Option<T> { + if ptr.is_null() { + None + } else { + Some(from_qemu_full(ptr)) + } + } +} + +impl FromQemuPtrNone<*const c_char> for String { + #[inline] + unsafe fn from_qemu_none(ptr: *const c_char) -> Self { + assert!(!ptr.is_null()); + String::from_utf8_lossy(CStr::from_ptr(ptr).to_bytes()).into_owned() + } +} + +impl FromQemuPtrFull<*mut c_char> for String { + #[inline] + unsafe fn from_qemu_full(ptr: *mut c_char) -> Self { + let res = from_qemu_none(ptr as *const _); + qemu_sys::g_free(ptr as *mut _); + res + } +} diff --git a/scripts/cargo.sh b/scripts/cargo.sh new file mode 100755 index 0000000000..bc000ef62c --- /dev/null +++ b/scripts/cargo.sh @@ -0,0 +1,29 @@ +#!/bin/sh + +export CARGO_CMD="$1" +shift + +if [ "$CARGO_CMD" = "build" ]; then + export MESON_BUILD_TYPE="$1" + shift + export MESON_CURRENT_BUILD_DIR="$1" + shift + export MESON_SOURCE_ROOT="$1" + shift + export MESON_BUILD_ROOT="$1" + shift +fi + +OUTDIR=debug + +if [[ "$MESON_BUILD_TYPE" = release ]] +then + EXTRA_ARGS="--release" + OUTDIR=release +fi + +cargo "$CARGO_CMD" --manifest-path "$MESON_SOURCE_ROOT/Cargo.toml" --target-dir="$MESON_BUILD_ROOT/rs-target" $EXTRA_ARGS "$@" + +if [ "$CARGO_CMD" = "build" ]; then + touch "$MESON_CURRENT_BUILD_DIR"/cargo-build.stamp +fi diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py index 4b03f7d53b..9743ea164b 100644 --- a/scripts/qapi-gen.py +++ b/scripts/qapi-gen.py @@ -17,10 +17,15 @@ from qapi.schema import QAPIError, QAPISchema from qapi.types import gen_types from qapi.visit import gen_visit +from qapi.rs_sys import gen_rs_systypes +from qapi.rs_types import gen_rs_types +from qapi.rs_dbus import gen_rs_dbus def main(argv): parser = argparse.ArgumentParser( description='Generate code from a QAPI schema') + parser.add_argument('-r', '--rust', action='store_true', + help="generate Rust code") parser.add_argument('-b', '--builtins', action='store_true', help="generate code for built-in types") parser.add_argument('-o', '--output-dir', action='store', default='', @@ -46,12 +51,17 @@ def main(argv): print(err, file=sys.stderr) exit(1) - gen_types(schema, args.output_dir, args.prefix, args.builtins) - gen_visit(schema, args.output_dir, args.prefix, args.builtins) - gen_commands(schema, args.output_dir, args.prefix) - gen_events(schema, args.output_dir, args.prefix) - gen_introspect(schema, args.output_dir, args.prefix, args.unmask) - gen_doc(schema, args.output_dir, args.prefix) + if args.rust: + gen_rs_systypes(schema, args.output_dir, args.prefix, args.builtins) + gen_rs_types(schema, args.output_dir, args.prefix, args.builtins) + gen_rs_dbus(schema, args.output_dir, args.prefix) + else: + gen_types(schema, args.output_dir, args.prefix, args.builtins) + gen_visit(schema, args.output_dir, args.prefix, args.builtins) + gen_commands(schema, args.output_dir, args.prefix) + gen_events(schema, args.output_dir, args.prefix) + gen_introspect(schema, args.output_dir, args.prefix, args.unmask) + gen_doc(schema, args.output_dir, args.prefix) if __name__ == '__main__': diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index ba35abea47..580b06c72a 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -42,6 +42,10 @@ def c_enum_const(type_name, const_name, prefix=None): return camel_to_upper(type_name) + '_' + c_name(const_name, False).upper() +def to_camel_case(value): + return ''.join(word.title() for word in filter(None, re.split("[-_]+", value))) + + c_name_trans = str.maketrans('.-', '__') diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 165925ca72..e998168ebe 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -342,6 +342,7 @@ class QAPIDoc: # servicable, but action at a distance. self._parser = parser self.info = info + self.text = '' # unprocessed text self.symbol = None self.body = QAPIDoc.Section() # dict mapping parameter name to ArgSection @@ -371,6 +372,7 @@ class QAPIDoc: * A features section: ._append_line is ._append_features_line * An additional section: ._append_line is ._append_various_line """ + self.text += line + '\n' line = line[1:] if not line: self._append_freeform(line) diff --git a/scripts/qapi/rs.py b/scripts/qapi/rs.py new file mode 100644 index 0000000000..7336df62e9 --- /dev/null +++ b/scripts/qapi/rs.py @@ -0,0 +1,199 @@ +""" +QAPI Rust generator +""" + +import os +import subprocess + +from qapi.common import * +from qapi.gen import QAPIGen, QAPISchemaVisitor + + +from_list = set() + + +def rs_name(name, protect=True): + name = c_name(name, protect) + if name == 'type': + name = "r#type" + return name + + +def rs_type(c_type, ns='qapi::', optional=False): + vec = False + to_rs = { + 'char': 'i8', + 'int8_t': 'i8', + 'uint8_t': 'u8', + 'int16_t': 'i16', + 'uint16_t': 'u16', + 'int32_t': 'i32', + 'uint32_t': 'u32', + 'int64_t': 'i64', + 'uint64_t': 'u64', + 'double': 'f64', + 'bool': 'bool', + 'str': 'String', + } + if c_type.startswith('const '): + c_type = c_type[6:] + if c_type.endswith(pointer_suffix): + c_type = c_type.rstrip(pointer_suffix).strip() + if c_type.endswith('List'): + c_type = c_type[:-4] + vec = True + else: + to_rs = { + 'char': 'String', + } + + if c_type in to_rs: + ret = to_rs[c_type] + else: + ret = ns + c_type + + if vec: + ret = 'Vec<%s>' % ret + if optional: + return 'Option<%s>' % ret + else: + return ret + + +def rs_systype(c_type, sys_ns='qapi_sys::'): + is_pointer = False + is_const = False + if c_type.endswith(pointer_suffix): + is_pointer = True + c_type = c_type.rstrip(pointer_suffix).strip() + + if c_type.startswith('const '): + c_type = c_type[6:] + is_const = True + + to_rs = { + 'char': 'libc::c_char', + 'int8_t': 'i8', + 'uint8_t': 'u8', + 'int16_t': 'i16', + 'uint16_t': 'u16', + 'int32_t': 'i32', + 'uint32_t': 'u32', + 'int64_t': 'libc::c_longlong', + 'uint64_t': 'libc::c_ulonglong', + 'double': 'libc::c_double', + 'bool': 'bool', + } + + rs = '' + if is_const and is_pointer: + rs += '*const ' + elif is_pointer: + rs += '*mut ' + if c_type in to_rs: + rs += to_rs[c_type] + else: + rs += sys_ns + c_type + + return rs + + +def build_params(arg_type, boxed, typefn=rs_systype, extra=[]): + ret = [] + if boxed: + assert arg_type + ret.append('arg: %s' % arg_type.c_param_type(const=True)) + elif arg_type: + assert not arg_type.variants + for memb in arg_type.members: + if memb.optional: + ret.append('has_%s: bool' % c_name(memb.name)) + ret.append('%s: %s' % (c_name(memb.name), typefn(memb.type.c_param_type(const=True)))) + ret.extend(extra) + return ', '.join(ret) + + +class QAPIGenRs(QAPIGen): + + def __init__(self, fname): + super().__init__(fname) + + +class QAPISchemaRsVisitor(QAPISchemaVisitor): + + def __init__(self, prefix, what): + self._prefix = prefix + self._what = what + self._gen = QAPIGenRs(self._prefix + self._what + '.rs') + + def write(self, output_dir): + self._gen.write(output_dir) + + pathname = os.path.join(output_dir, self._gen.fname) + subprocess.call(['rustfmt', pathname]) + + +def to_qemu_none(c_type, name): + is_pointer = False + is_const = False + if c_type.endswith(pointer_suffix): + is_pointer = True + c_type = c_type.rstrip(pointer_suffix).strip() + sys_type = rs_systype(c_type) + + if c_type.startswith('const '): + c_type = c_type[6:] + is_const = True + + if is_pointer: + if c_type == 'char': + return mcgen(''' + let %(name)s_ = CString::new(%(name)s).unwrap(); + let %(name)s = %(name)s_.as_ptr(); +''', name=name) + else: + return mcgen(''' + let %(name)s_ = %(name)s.to_qemu_none(); + let %(name)s = %(name)s_.0; +''', name=name, sys_type=sys_type) + return '' + + +def gen_call(name, arg_type, boxed, ret_type): + ret = '' + + argstr = '' + if boxed: + assert arg_type + argstr = '&arg, ' + elif arg_type: + assert not arg_type.variants + for memb in arg_type.members: + if memb.optional: + argstr += 'has_%s, ' % c_name(memb.name) + ret += to_qemu_none(memb.type.c_type(), c_name(memb.name)) + argstr += ' %s, ' % c_name(memb.name) + + lhs = '' + if ret_type: + lhs = 'let retval_ = ' + + ret += mcgen(''' + +%(lhs)sqmp_%(c_name)s(%(args)s&mut err_); +''', + c_name=c_name(name), args=argstr, lhs=lhs) + return ret + + +def from_qemu(var_name, c_type, full=False): + if c_type.endswith('List' + pointer_suffix): + from_list.add(c_type) + is_pointer = c_type.endswith(pointer_suffix) + if is_pointer: + if full: + return 'from_qemu_full(%s as *mut _)' % var_name + else: + return 'from_qemu_none(%s as *const _)' % var_name + else: + return var_name diff --git a/scripts/qapi/rs_dbus.py b/scripts/qapi/rs_dbus.py new file mode 100644 index 0000000000..5036e774a8 --- /dev/null +++ b/scripts/qapi/rs_dbus.py @@ -0,0 +1,86 @@ +""" +QAPI Rust DBus interface generator +""" + +from qapi.common import * +from qapi.rs import QAPISchemaRsVisitor, rs_systype, from_qemu, build_params, rs_type, gen_call + + +class QAPISchemaGenRsDBusVisitor(QAPISchemaRsVisitor): + + def __init__(self, prefix): + super().__init__(prefix, 'qapi-dbus') + self._cur_doc = None + self._dbus_gen = '' + + def visit_begin(self, schema): + self.schema = schema + self._gen.add( + mcgen(''' +// generated by qapi-gen, DO NOT EDIT + +extern "C" { +''')) + + def visit_end(self): + self._gen.add(mcgen(''' +} + +pub(crate) struct %(name)s; + +#[dbus_interface(name = "org.qemu.%(name)s")] +impl %(name)s { +%(dbus)s +} +''', name=to_camel_case(self._prefix + 'Qapi'), dbus=self._dbus_gen)) + + def visit_command(self, name, info, ifcond, features, + arg_type, ret_type, gen, success_response, boxed, + allow_oob, allow_preconfig): + if not gen: + return + + ret = '' + retval = '()' + if ret_type: + ret = ' -> %s' % rs_systype(ret_type.c_type()) + retval = from_qemu('retval_', ret_type.c_type(), full=True) + + doc = '' + for s in self.schema.docs: + if s.symbol == name: + for l in s.text.splitlines(): + doc += '///%s\n' % l[1:] + if doc.endswith('\n'): + doc = doc[:-1] + + self._gen.add(mcgen(''' + fn qmp_%(name)s(%(params)s)%(ret)s; +''', name = c_name(name), params=build_params(arg_type, boxed, extra=['errp: *mut *mut qemu_sys::Error']), ret=ret)) + + ret = ' -> Result<()>' + if ret_type: + ret = ' -> Result<%s>' % rs_type(ret_type.c_type()) + + self._dbus_gen += mcgen(''' + %(doc)s + fn %(name)s(&self, %(params)s)%(ret)s { + unsafe { + let mut err_ = ptr::null_mut(); + %(call)s + if err_.is_null() { + Ok(%(retval)s) + } else { + Err(from_qemu_full(err_)) + } + } + } + +''', doc = doc, call = gen_call(name, arg_type, boxed, ret_type), + name = c_name(name), params=build_params(arg_type, boxed, typefn=rs_type), ret=ret, retval=retval) + + +def gen_rs_dbus(schema, output_dir, prefix): + vis = QAPISchemaGenRsDBusVisitor(prefix) + schema.visit(vis) + vis.write(output_dir) diff --git a/scripts/qapi/rs_sys.py b/scripts/qapi/rs_sys.py new file mode 100644 index 0000000000..5128398cb9 --- /dev/null +++ b/scripts/qapi/rs_sys.py @@ -0,0 +1,183 @@ +""" +QAPI Rust sys/ffi generator +""" + +from qapi.common import * +from qapi.schema import QAPISchemaEnumMember, QAPISchemaObjectType +from qapi.rs import QAPISchemaRsVisitor, rs_systype, rs_name + + +objects_seen = set() + + +def gen_rs_sys_enum(name, members, prefix=None): + # append automatically generated _max value + enum_members = members + [QAPISchemaEnumMember('_MAX', None)] + + ret = mcgen(''' + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +#[cfg_attr(feature = "dbus", derive(Deserialize_repr, Serialize_repr, Type))] +#[repr(i32)] // FIXME: serde-repr#10 +pub enum %(rs_name)s { +''', + rs_name=rs_name(name)) + + for m in enum_members: + ret += mcgen(''' + %(c_enum)s, +''', + c_enum=to_camel_case(rs_name(m.name, False))) + ret += mcgen(''' +} +''') + return ret + + +def gen_rs_sys_struct_members(members): + ret = '' + for memb in members: + if memb.optional: + ret += mcgen(''' + pub has_%(rs_name)s: bool, +''', + rs_name=rs_name(memb.name)) + ret += mcgen(''' + pub %(rs_name)s: %(rs_systype)s, +''', + rs_systype=rs_systype(memb.type.c_type(), ''), rs_name=rs_name(memb.name)) + return ret + + +def gen_rs_sys_free(ty): + return mcgen(''' + +extern "C" { + pub fn qapi_free_%(ty)s(obj: *mut %(ty)s); +} +''', ty=ty) + + +def gen_rs_sys_object(name, ifcond, base, members, variants): + if name in objects_seen: + return '' + + ret = '' + objects_seen.add(name) + if variants: + ret += 'variants TODO' + + ret += gen_rs_sys_free(rs_name(name)) + ret += mcgen(''' + +#[repr(C)] +#[derive(Debug)] +pub struct %(rs_name)s { +''', + rs_name=rs_name(name)) + + if base: + ret += 'Base TODO' + + ret += gen_rs_sys_struct_members(members) + + ret += mcgen(''' +} +''') + return ret + + +def gen_rs_sys_variant(name, ifcond, variants): + if name in objects_seen: + return '' + + objects_seen.add(name) + + vs = '' + for var in variants.variants: + if var.type.name == 'q_empty': + continue + vs += mcgen(''' + pub %(mem_name)s: %(rs_systype)s, +''', + rs_systype=rs_systype(var.type.c_unboxed_type(), ''), + mem_name=rs_name(var.name)) + + return mcgen(''' + +#[repr(C)] +pub union %(rs_name)sUnion { + %(variants)s +} + +#[repr(C)] +pub struct %(rs_name)s { + pub ty: QType, + pub u: %(rs_name)sUnion, +} +''', + rs_name=rs_name(name), variants=vs) + + +def gen_rs_sys_array(name, element_type): + ret = mcgen(''' + +#[repr(C)] +pub struct %(rs_name)s { + pub next: *mut %(rs_name)s, + pub value: %(rs_systype)s, +} + +impl ::std::fmt::Debug for %(rs_name)s { + fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result { + f.debug_struct(&format!("%(rs_name)s @ {:?}", self as *const _)) + .finish() + } +} +''', + rs_name=rs_name(name), rs_systype=rs_systype(element_type.c_type(), '')) + ret += gen_rs_sys_free(rs_name(name)) + return ret + + +class QAPISchemaGenRsSysTypeVisitor(QAPISchemaRsVisitor): + + def __init__(self, prefix): + super().__init__(prefix, 'qapi-sys-types') + + def visit_begin(self, schema): + # gen_object() is recursive, ensure it doesn't visit the empty type + objects_seen.add(schema.the_empty_object_type.name) + self._gen.preamble_add( + mcgen(''' +// generated by qapi-gen, DO NOT EDIT + +#[cfg(feature = "dbus")] +use serde_repr::{Deserialize_repr, Serialize_repr}; +#[cfg(feature = "dbus")] +use zvariant_derive::Type; + +use crate::qemu_sys::{QNull, QObject}; + +''')) + + def visit_enum_type(self, name, info, ifcond, features, members, prefix): + self._gen.add(gen_rs_sys_enum(name, members, prefix)) + + def visit_array_type(self, name, info, ifcond, element_type): + self._gen.add(gen_rs_sys_array(name, element_type)) + + def visit_object_type(self, name, info, ifcond, features, + base, members, variants): + if name.startswith('q_'): + return + self._gen.add(gen_rs_sys_object(name, ifcond, base, members, variants)) + + def visit_alternate_type(self, name, info, ifcond, features, variants): + self._gen.add(gen_rs_sys_variant(name, ifcond, variants)) + + +def gen_rs_systypes(schema, output_dir, prefix, opt_builtins): + vis = QAPISchemaGenRsSysTypeVisitor(prefix) + schema.visit(vis) + vis.write(output_dir) diff --git a/scripts/qapi/rs_types.py b/scripts/qapi/rs_types.py new file mode 100644 index 0000000000..8b22605bf9 --- /dev/null +++ b/scripts/qapi/rs_types.py @@ -0,0 +1,281 @@ +""" +QAPI Rust types generator +""" + +from qapi.common import * +from qapi.rs import QAPISchemaRsVisitor, rs_systype, rs_name, from_qemu, rs_type, from_list + + +objects_seen = set() + + +def gen_rs_object(name, ifcond, base, members, variants): + if name in objects_seen: + return '' + + ret = '' + objects_seen.add(name) + has_options = False + for memb in members: + if memb.optional: + has_options = True + + if variants: + ret += 'variants TODO' + + derive = 'Serialize, Deserialize, Type' + serde = 'serde' + if has_options: + derive = 'SerializeDict, DeserializeDict, TypeDict' + serde = 'zvariant' + + ret += mcgen(''' + +#[derive(Clone, Debug)] +#[cfg_attr(feature = "dbus", derive(%(derive)s))] +pub struct %(rs_name)s { +''', + rs_name=rs_name(name), derive=derive) + + if base: + ret += 'Base TODO' + + memb_names = [] + for memb in members: + memb_names.append(rs_name(memb.name)) + rsname = rs_name(memb.name) + if rsname != memb.name: + ret += mcgen(''' + #[cfg_attr(feature = "dbus", %(serde)s(rename = "%(name)s"))] +''', serde=serde, name=memb.name) + + ret += mcgen(''' + pub %(rs_name)s: %(rs_type)s, +''', + rs_type=rs_type(memb.type.c_type(), '', optional=memb.optional), rs_name=rsname) + + ret += mcgen(''' +} + +impl FromQemuPtrFull<*mut qapi_sys::%(rs_name)s> for %(rs_name)s { + unsafe fn from_qemu_full(sys: *mut qapi_sys::%(rs_name)s) -> Self { + let ret = from_qemu_none(sys as *const _); + qapi_sys::qapi_free_%(rs_name)s(sys); + ret + } +} + +impl FromQemuPtrNone<*const qapi_sys::%(rs_name)s> for %(rs_name)s { + unsafe fn from_qemu_none(sys: *const qapi_sys::%(rs_name)s) -> Self { + let sys = & *sys; +''', rs_name=rs_name(name)) + + for memb in members: + memb_name = rs_name(memb.name) + val = from_qemu('sys.' + memb_name, memb.type.c_type()) + if memb.optional: + val = mcgen('''{ + if sys.has_%(memb_name)s { + Some(%(val)s) + } else { + None + } +}''', memb_name=memb_name, val=val) + + ret += mcgen(''' + let %(memb_name)s = %(val)s; +''', memb_name=memb_name, val=val) + + ret += mcgen(''' + Self { %(memb_names)s } + } +} +''', rs_name=rs_name(name), memb_names=', '.join(memb_names)) + + storage = [] + stash = [] + sys_memb = [] + memb_none = '' + memb_full = '' + for memb in members: + memb_name = rs_name(memb.name) + c_type = memb.type.c_type() + is_pointer = c_type.endswith(pointer_suffix) + if is_pointer: + t = rs_type(memb.type.c_type(), optional=memb.optional, ns='') + p = rs_systype(memb.type.c_type()) + s = "translate::Stash<'a, %s, %s>" % (p, t) + storage.append(s) + if memb.optional: + sys_memb.append('has_%s' % memb_name) + has_memb = mcgen(''' + let has_%(memb_name)s = self.%(memb_name)s.is_some(); +''', memb_name=memb_name) + memb_none += has_memb + memb_full += has_memb + + to_qemu = '' + if is_pointer: + memb_none += mcgen(''' + let %(memb_name)s_stash_ = self.%(memb_name)s.to_qemu_none(); + let %(memb_name)s = %(memb_name)s_stash_.0; +''', memb_name=memb_name) + stash.append('%s_stash_' % memb_name) + memb_full += mcgen(''' + let %(memb_name)s = self.%(memb_name)s.to_qemu_full(); +''', memb_name=memb_name) + else: + unwrap = '' + if memb.optional: + unwrap = '.unwrap_or_default()' + memb = mcgen(''' + let %(memb_name)s = self.%(memb_name)s%(unwrap)s; +''', memb_name=memb_name, unwrap=unwrap) + memb_none += memb + memb_full += memb + + sys_memb.append(memb_name) + + ret += mcgen(''' + +impl translate::QemuPtrDefault for %(rs_name)s { + type QemuType = *mut qapi_sys::%(rs_name)s; +} + +impl<'a> translate::ToQemuPtr<'a, *mut qapi_sys::%(rs_name)s> for %(rs_name)s { + type Storage = (Box<qapi_sys::%(rs_name)s>, %(storage)s); + + #[inline] + fn to_qemu_none(&'a self) -> translate::Stash<'a, *mut qapi_sys::%(rs_name)s, %(rs_name)s> { + %(memb_none)s + let mut box_ = Box::new(qapi_sys::%(rs_name)s { %(sys_memb)s }); + + translate::Stash(&mut *box_, (box_, %(stash)s)) + } + + #[inline] + fn to_qemu_full(&self) -> *mut qapi_sys::%(rs_name)s { + unsafe { + %(memb_full)s + let ptr = qemu_sys::g_malloc0(std::mem::size_of::<*const %(rs_name)s>()) as *mut _; + *ptr = qapi_sys::%(rs_name)s { %(sys_memb)s }; + ptr + } + } +} +''', rs_name=rs_name(name), storage=', '.join(storage), + sys_memb=', '.join(sys_memb), memb_none=memb_none, memb_full=memb_full, stash=', '.join(stash)) + + return ret + + +def gen_rs_variant(name, ifcond, variants): + if name in objects_seen: + return '' + + ret = '' + objects_seen.add(name) + + ret += mcgen(''' + +// Implement manual Value conversion (other option via some zbus macros?) +#[cfg(feature = "dbus")] +impl zvariant::Type for %(rs_name)s { + fn signature() -> zvariant::Signature<'static> { + zvariant::Value::signature() + } +} + +#[derive(Clone,Debug)] +#[cfg_attr(feature = "dbus", derive(Deserialize, Serialize), serde(into = "zvariant::OwnedValue", try_from = "zvariant::OwnedValue"))] +pub enum %(rs_name)s { +''', + rs_name=rs_name(name)) + + for var in variants.variants: + if var.type.name == 'q_empty': + continue + ret += mcgen(''' + %(mem_name)s(%(rs_type)s), +''', + rs_type=rs_type(var.type.c_unboxed_type(), ''), + mem_name=to_camel_case(rs_name(var.name))) + ret += mcgen(''' +} +''') + return ret + + +class QAPISchemaGenRsTypeVisitor(QAPISchemaRsVisitor): + + def __init__(self, prefix): + super().__init__(prefix, 'qapi-types') + + def visit_begin(self, schema): + # gen_object() is recursive, ensure it doesn't visit the empty type + objects_seen.add(schema.the_empty_object_type.name) + self._gen.preamble_add( + mcgen(''' +// generated by qapi-gen, DO NOT EDIT +''')) + + def visit_end(self): + for c_type in from_list: + sys = rs_systype(c_type, sys_ns='')[5:] + rs = rs_type(c_type, ns='') + + self._gen.add(mcgen(''' + +impl FromQemuPtrFull<*mut qapi_sys::%(sys)s> for %(rs)s { + #[inline] + unsafe fn from_qemu_full(sys: *mut qapi_sys::%(sys)s) -> Self { + let ret = from_qemu_none(sys as *const _); + qapi_sys::qapi_free_%(sys)s(sys); + ret + } +} + +impl FromQemuPtrNone<*const qapi_sys::%(sys)s> for %(rs)s { + #[inline] + unsafe fn from_qemu_none(sys: *const qapi_sys::%(sys)s) -> Self { + let mut ret = vec![]; + let mut it = sys; + while !it.is_null() { + let e = &*it; + ret.push(translate::from_qemu_none(e.value as *const _)); + it = e.next; + } + ret + } +} +''', sys=sys, rs=rs)) + + def visit_command(self, name, info, ifcond, features, + arg_type, ret_type, gen, success_response, boxed, + allow_oob, allow_preconfig): + if not gen: + return + # hack: eventually register a from_list + if ret_type: + from_qemu('', ret_type.c_type()) + + def visit_object_type(self, name, info, ifcond, features, + base, members, variants): + if name.startswith('q_'): + return + self._gen.add(gen_rs_object(name, ifcond, base, members, variants)) + + def visit_enum_type(self, name, info, ifcond, features, members, prefix): + self._gen.add(mcgen(''' + +pub type %(rs_name)s = qapi_sys::%(rs_name)s; +''', rs_name=rs_name(name))) + + def visit_alternate_type(self, name, info, ifcond, features, variants): + self._gen.add(gen_rs_variant(name, ifcond, variants)) + + +def gen_rs_types(schema, output_dir, prefix, opt_builtins): + vis = QAPISchemaGenRsTypeVisitor(prefix) + schema.visit(vis) + vis.write(output_dir) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 78309a00f0..da48210509 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -167,8 +167,14 @@ class QAPISchemaType(QAPISchemaEntity): pass # Return the C type to be used in a parameter list. - def c_param_type(self): - return self.c_type() + # + # The argument should be considered const, since no ownership is given to the callee, + # but qemu C code frequently tweaks it. Set const=True for a stricter declaration. + def c_param_type(self, const=False): + c_type = self.c_type() + if const and c_type.endswith(pointer_suffix): + c_type = 'const ' + c_type + return c_type # Return the C type to be used where we suppress boxing. def c_unboxed_type(self): @@ -221,10 +227,10 @@ class QAPISchemaBuiltinType(QAPISchemaType): def c_type(self): return self._c_type_name - def c_param_type(self): + def c_param_type(self, const=False): if self.name == 'str': return 'const ' + self._c_type_name - return self._c_type_name + return super().c_param_type(const) def json_type(self): return self._json_type_name