| Message ID | 1349372021-31212-22-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Il 04/10/2012 19:33, Michael Roth ha scritto: > +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) \ > + g_assert(qidl_data_##name.schema_obj); \ > + object_property_add_link(obj, path, "container", \ Why "container" as the type? Paolo > + &qidl_data_##name.schema_obj, errp) > +
On Fri, Oct 05, 2012 at 10:14:09AM +0200, Paolo Bonzini wrote: > Il 04/10/2012 19:33, Michael Roth ha scritto: > > +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) \ > > + g_assert(qidl_data_##name.schema_obj); \ > > + object_property_add_link(obj, path, "container", \ > > Why "container" as the type? I needed a path in the QOM tree where I can stick the json schema property, so I just used a "container" object, which is what this link points to. Don't have any plans on using the object for anything beyond that atm, so didn't really make sense to me to define a new object type. > > Paolo > > > + &qidl_data_##name.schema_obj, errp) > > + > >
Il 04/10/2012 19:33, Michael Roth ha scritto: > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > qidl.h | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 113 insertions(+) > create mode 100644 qidl.h > > diff --git a/qidl.h b/qidl.h > new file mode 100644 > index 0000000..eae0202 > --- /dev/null > +++ b/qidl.h > @@ -0,0 +1,113 @@ > +/* > + * QEMU IDL Macros/stubs > + * > + * See docs/qidl.txt for usage information. > + * > + * Copyright IBM, Corp. 2012 > + * > + * Authors: > + * Michael Roth <mdroth@linux.vnet.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPLv2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ > + > +#ifndef QIDL_H > +#define QIDL_H > + > +#include <glib.h> > +#include "qapi/qapi-visit-core.h" > +#include "qemu/object.h" > +#include "hw/qdev-properties.h" > + > +#ifdef QIDL_GEN > + > +/* we pass the code through the preprocessor with QIDL_GEN defined to parse > + * structures as they'd appear after preprocessing, and use the following > + * definitions mostly to re-insert the initial macros/annotations so they > + * stick around for the parser to process > + */ > +#define QIDL(...) QIDL(__VA_ARGS__) > +#define QIDL_START(name, ...) QIDL_START(name, ##__VA_ARGS__) > + > +#define QIDL_VISIT_TYPE(name, v, s, f, e) > +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) > +#define QIDL_PROPERTIES(name) Ok, a few questions... Why do you need these to expand to nothing in the QIDL_GEN case? > +#define QIDL_DECLARE(name, ...) \ Can QIDL_DECLARE replace QIDL_ENABLED as the magic detection string for qidl compilation? > + QIDL_START(name, ##__VA_ARGS__) \ > + struct name > + > +#else /* !QIDL_GEN */ > + > +#define QIDL(...) > +#ifdef QIDL_ENABLED > +#define QIDL_START(name, ...) \ > + static struct { \ > + void (*visitor)(Visitor *, struct name **, const char *, Error **); \ > + const char *schema_json_text; \ > + Object *schema_obj; \ > + Property *properties; \ > + } qidl_data_##name; > +#else > +#define QIDL_START(name, ...) > +#endif > + > +#define QIDL_DECLARE(name, ...) \ > + QIDL_START(name, ##__VA_ARGS__) \ > + struct name This is the same definition as in the QIDL_GEN case. Please include it just once. > +#define QIDL_VISIT_TYPE(name, v, s, f, e) \ > + g_assert(qidl_data_##name.visitor); \ > + qidl_data_##name.visitor(v, s, NULL, e) > + > +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) \ > + g_assert(qidl_data_##name.schema_obj); \ > + object_property_add_link(obj, path, "container", \ > + &qidl_data_##name.schema_obj, errp) Where will QIDL_SCHEMA_ADD_LINK be used? > +#define QIDL_PROPERTIES(name) \ > + qidl_data_##name.properties > + > +#endif /* QIDL_GEN */ > + > +/* must be "called" in any C files that make use of QIDL-generated code */ > +#define QIDL_ENABLE() > + > +/* QIDL annotations/markers > + * > + * q_immutable: state is fully restorable via device > + * [re-]initialization/realization > + * > + * q_derived: state can be fully reconstructed from other fields (and will be, > + * via [re-]initialization of the device or a separate hook) > + * > + * q_broken: state should (or possibly should) be saved, but isn't. mostly an aid > + * for device developers having issues with serialization of a particular > + * field, committed code should contain these except in special circumstances > + * > + * q_optional: <field> should only be serialized if the field by the name of > + * has_<field> is true > + * > + * q_elsewhere: state should be serialized, but is done so elsewhere (for > + * instance, by another device with a pointer to the same data) > + * > + * q_size(field): for static/dynamically-allocated arrays. specifies the field > + * in the structure containing the number of elements that should be > + * serialized. if argument is wrapped in parenthesis it is instead interpreted > + * as an expression that should be invaluated to determine the size. > + * > + * q_property(<property name> [, <default value>]): specifies that field is a > + * qdev-style property. all properties of the struct are then accessible via > + * QIDL_PROPERTIES(<device name>) macro. > + */ > + > +#define q_immutable QIDL(immutable) > +#define q_derived QIDL(derived) > +#define q_broken QIDL(broken) > +#define q_optional QIDL(optional) > +#define q_elsewhere QIDL(elsewhere) > +#define q_size(...) QIDL(size_is, ##__VA_ARGS__) > +#define q_property(name, ...) QIDL(property, name, ##__VA_ARGS__) > + > +#endif >
On Fri, Oct 05, 2012 at 05:07:46PM +0200, Paolo Bonzini wrote: > Il 04/10/2012 19:33, Michael Roth ha scritto: > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > --- > > qidl.h | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 113 insertions(+) > > create mode 100644 qidl.h > > > > diff --git a/qidl.h b/qidl.h > > new file mode 100644 > > index 0000000..eae0202 > > --- /dev/null > > +++ b/qidl.h > > @@ -0,0 +1,113 @@ > > +/* > > + * QEMU IDL Macros/stubs > > + * > > + * See docs/qidl.txt for usage information. > > + * > > + * Copyright IBM, Corp. 2012 > > + * > > + * Authors: > > + * Michael Roth <mdroth@linux.vnet.ibm.com> > > + * > > + * This work is licensed under the terms of the GNU GPLv2 or later. > > + * See the COPYING file in the top-level directory. > > + * > > + */ > > + > > +#ifndef QIDL_H > > +#define QIDL_H > > + > > +#include <glib.h> > > +#include "qapi/qapi-visit-core.h" > > +#include "qemu/object.h" > > +#include "hw/qdev-properties.h" > > + > > +#ifdef QIDL_GEN > > + > > +/* we pass the code through the preprocessor with QIDL_GEN defined to parse > > + * structures as they'd appear after preprocessing, and use the following > > + * definitions mostly to re-insert the initial macros/annotations so they > > + * stick around for the parser to process > > + */ > > +#define QIDL(...) QIDL(__VA_ARGS__) > > +#define QIDL_START(name, ...) QIDL_START(name, ##__VA_ARGS__) > > + > > +#define QIDL_VISIT_TYPE(name, v, s, f, e) > > +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) > > +#define QIDL_PROPERTIES(name) > > Ok, a few questions... > > Why do you need these to expand to nothing in the QIDL_GEN case? > They don't need to, I was just trying to be explicit about what directives were relevant to the parser and which ones were relevant to the actually compiled code. It was more a development "aid" than anything else though, so I think we can drop the special handling and clean these up a bit. > > +#define QIDL_DECLARE(name, ...) \ > > Can QIDL_DECLARE replace QIDL_ENABLED as the magic detection string for > qidl compilation? > In some cases the declarations will come via #include'd headers, so the only way to do that reliable is to run it through the preprocessor first, which is how things were done in v1. But running everything through cpp adds substantial overhead, and just because a QIDL-fied struct is included in a C file, it doesn't mean that the C file intends to use any qidl-generated code. So that's why the QIDL_ENABLE() directive was added, so we can scan for that before doing the pass through cpp and the qidl parser to process all the qidl declarations. > > + QIDL_START(name, ##__VA_ARGS__) \ > > + struct name > > + > > +#else /* !QIDL_GEN */ > > + > > +#define QIDL(...) > > +#ifdef QIDL_ENABLED > > +#define QIDL_START(name, ...) \ > > + static struct { \ > > + void (*visitor)(Visitor *, struct name **, const char *, Error **); \ > > + const char *schema_json_text; \ > > + Object *schema_obj; \ > > + Property *properties; \ > > + } qidl_data_##name; > > +#else > > +#define QIDL_START(name, ...) > > +#endif > > + > > +#define QIDL_DECLARE(name, ...) \ > > + QIDL_START(name, ##__VA_ARGS__) \ > > + struct name > > This is the same definition as in the QIDL_GEN case. Please include it > just once. > Will do. > > +#define QIDL_VISIT_TYPE(name, v, s, f, e) \ > > + g_assert(qidl_data_##name.visitor); \ > > + qidl_data_##name.visitor(v, s, NULL, e) > > + > > +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) \ > > + g_assert(qidl_data_##name.schema_obj); \ > > + object_property_add_link(obj, path, "container", \ > > + &qidl_data_##name.schema_obj, errp) > > Where will QIDL_SCHEMA_ADD_LINK be used? In general, in the init functions of any qom objects that expose a "state" property in the qom tree, so that in every case where we expose serialized state information for an object/device via qom, we also provide a link to a schema describing it's structure. > > > +#define QIDL_PROPERTIES(name) \ > > + qidl_data_##name.properties > > + > > +#endif /* QIDL_GEN */ > > + > > +/* must be "called" in any C files that make use of QIDL-generated code */ > > +#define QIDL_ENABLE() > > + > > +/* QIDL annotations/markers > > + * > > + * q_immutable: state is fully restorable via device > > + * [re-]initialization/realization > > + * > > + * q_derived: state can be fully reconstructed from other fields (and will be, > > + * via [re-]initialization of the device or a separate hook) > > + * > > + * q_broken: state should (or possibly should) be saved, but isn't. mostly an aid > > + * for device developers having issues with serialization of a particular > > + * field, committed code should contain these except in special circumstances > > + * > > + * q_optional: <field> should only be serialized if the field by the name of > > + * has_<field> is true > > + * > > + * q_elsewhere: state should be serialized, but is done so elsewhere (for > > + * instance, by another device with a pointer to the same data) > > + * > > + * q_size(field): for static/dynamically-allocated arrays. specifies the field > > + * in the structure containing the number of elements that should be > > + * serialized. if argument is wrapped in parenthesis it is instead interpreted > > + * as an expression that should be invaluated to determine the size. > > + * > > + * q_property(<property name> [, <default value>]): specifies that field is a > > + * qdev-style property. all properties of the struct are then accessible via > > + * QIDL_PROPERTIES(<device name>) macro. > > + */ > > + > > +#define q_immutable QIDL(immutable) > > +#define q_derived QIDL(derived) > > +#define q_broken QIDL(broken) > > +#define q_optional QIDL(optional) > > +#define q_elsewhere QIDL(elsewhere) > > +#define q_size(...) QIDL(size_is, ##__VA_ARGS__) > > +#define q_property(name, ...) QIDL(property, name, ##__VA_ARGS__) > > + > > +#endif > > > >
Il 05/10/2012 17:41, Michael Roth ha scritto: > On Fri, Oct 05, 2012 at 05:07:46PM +0200, Paolo Bonzini wrote: >> Il 04/10/2012 19:33, Michael Roth ha scritto: >>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> >>> --- >>> qidl.h | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 113 insertions(+) >>> create mode 100644 qidl.h >>> >>> diff --git a/qidl.h b/qidl.h >>> new file mode 100644 >>> index 0000000..eae0202 >>> --- /dev/null >>> +++ b/qidl.h >>> @@ -0,0 +1,113 @@ >>> +/* >>> + * QEMU IDL Macros/stubs >>> + * >>> + * See docs/qidl.txt for usage information. >>> + * >>> + * Copyright IBM, Corp. 2012 >>> + * >>> + * Authors: >>> + * Michael Roth <mdroth@linux.vnet.ibm.com> >>> + * >>> + * This work is licensed under the terms of the GNU GPLv2 or later. >>> + * See the COPYING file in the top-level directory. >>> + * >>> + */ >>> + >>> +#ifndef QIDL_H >>> +#define QIDL_H >>> + >>> +#include <glib.h> >>> +#include "qapi/qapi-visit-core.h" >>> +#include "qemu/object.h" >>> +#include "hw/qdev-properties.h" >>> + >>> +#ifdef QIDL_GEN >>> + >>> +/* we pass the code through the preprocessor with QIDL_GEN defined to parse >>> + * structures as they'd appear after preprocessing, and use the following >>> + * definitions mostly to re-insert the initial macros/annotations so they >>> + * stick around for the parser to process >>> + */ >>> +#define QIDL(...) QIDL(__VA_ARGS__) >>> +#define QIDL_START(name, ...) QIDL_START(name, ##__VA_ARGS__) >>> + >>> +#define QIDL_VISIT_TYPE(name, v, s, f, e) >>> +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) >>> +#define QIDL_PROPERTIES(name) >> >> Ok, a few questions... >> >> Why do you need these to expand to nothing in the QIDL_GEN case? >> > > They don't need to, I was just trying to be explicit about what > directives were relevant to the parser and which ones were relevant to > the actually compiled code. It was more a development "aid" than > anything else though, so I think we can drop the special handling and > clean these up a bit. Yes, thanks! >>> +#define QIDL_DECLARE(name, ...) \ >> >> Can QIDL_DECLARE replace QIDL_ENABLED as the magic detection string for >> qidl compilation? >> > > In some cases the declarations will come via #include'd headers, so the > only way to do that reliable is to run it through the preprocessor > first, which is how things were done in v1. But running everything > through cpp adds substantial overhead, and just because a QIDL-fied > struct is included in a C file, it doesn't mean that the C file intends > to use any qidl-generated code. Ok, I guess I need to see some example. We can clean it up later if we find a more clever way to do things. Paolo
On Fri, Oct 05, 2012 at 05:53:09PM +0200, Paolo Bonzini wrote: > Il 05/10/2012 17:41, Michael Roth ha scritto: > > On Fri, Oct 05, 2012 at 05:07:46PM +0200, Paolo Bonzini wrote: > >> Il 04/10/2012 19:33, Michael Roth ha scritto: > >>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > >>> --- > >>> qidl.h | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 113 insertions(+) > >>> create mode 100644 qidl.h > >>> > >>> diff --git a/qidl.h b/qidl.h > >>> new file mode 100644 > >>> index 0000000..eae0202 > >>> --- /dev/null > >>> +++ b/qidl.h > >>> @@ -0,0 +1,113 @@ > >>> +/* > >>> + * QEMU IDL Macros/stubs > >>> + * > >>> + * See docs/qidl.txt for usage information. > >>> + * > >>> + * Copyright IBM, Corp. 2012 > >>> + * > >>> + * Authors: > >>> + * Michael Roth <mdroth@linux.vnet.ibm.com> > >>> + * > >>> + * This work is licensed under the terms of the GNU GPLv2 or later. > >>> + * See the COPYING file in the top-level directory. > >>> + * > >>> + */ > >>> + > >>> +#ifndef QIDL_H > >>> +#define QIDL_H > >>> + > >>> +#include <glib.h> > >>> +#include "qapi/qapi-visit-core.h" > >>> +#include "qemu/object.h" > >>> +#include "hw/qdev-properties.h" > >>> + > >>> +#ifdef QIDL_GEN > >>> + > >>> +/* we pass the code through the preprocessor with QIDL_GEN defined to parse > >>> + * structures as they'd appear after preprocessing, and use the following > >>> + * definitions mostly to re-insert the initial macros/annotations so they > >>> + * stick around for the parser to process > >>> + */ > >>> +#define QIDL(...) QIDL(__VA_ARGS__) > >>> +#define QIDL_START(name, ...) QIDL_START(name, ##__VA_ARGS__) > >>> + > >>> +#define QIDL_VISIT_TYPE(name, v, s, f, e) > >>> +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) > >>> +#define QIDL_PROPERTIES(name) > >> > >> Ok, a few questions... > >> > >> Why do you need these to expand to nothing in the QIDL_GEN case? > >> > > > > They don't need to, I was just trying to be explicit about what > > directives were relevant to the parser and which ones were relevant to > > the actually compiled code. It was more a development "aid" than > > anything else though, so I think we can drop the special handling and > > clean these up a bit. > > Yes, thanks! > > >>> +#define QIDL_DECLARE(name, ...) \ > >> > >> Can QIDL_DECLARE replace QIDL_ENABLED as the magic detection string for > >> qidl compilation? > >> > > > > In some cases the declarations will come via #include'd headers, so the > > only way to do that reliable is to run it through the preprocessor > > first, which is how things were done in v1. But running everything > > through cpp adds substantial overhead, and just because a QIDL-fied > > struct is included in a C file, it doesn't mean that the C file intends > > to use any qidl-generated code. > > Ok, I guess I need to see some example. We can clean it up later if we > find a more clever way to do things. This was the main example I hit (not yet rebased): https://github.com/mdroth/qemu/commit/d8ea7c7a882e2fcbd0a9b7ab9ea47a389f87d31b As part of that patch We add annotations to PCIDevice in pci.h, which then gets pulled in from quite a few devices. So we end up with *.qidl.c files for devices that don't expose a "state" property or even have a QIDL_DECLARE() directive. If we were to scan for QIDL_DECLARE() in advance of running it through the preprocessor, we'd address a lot of those case. But then we miss cases like this: https://github.com/mdroth/qemu/commit/2199f721daebd5c3961069bdd51de80a5b4fa827 where, in pci.c, we use code generated from declarations in pci_internals.h even though pci.c doesn't contain a QIDL_DECLARE() We could in theory scan for QIDL_PROPERTIES()/QIDL_SCHEMA_ADD_LINK()/QIDL_VISIT_TYPE() to avoid the need for QIDL_ENABLE(), but to me the latter approach seemed like it would scale better if we were to leverage QIDL for other things in the future. > > Paolo >
Il 05/10/2012 18:47, Michael Roth ha scritto: > On Fri, Oct 05, 2012 at 05:53:09PM +0200, Paolo Bonzini wrote: >> Il 05/10/2012 17:41, Michael Roth ha scritto: >>> On Fri, Oct 05, 2012 at 05:07:46PM +0200, Paolo Bonzini wrote: >>>> Il 04/10/2012 19:33, Michael Roth ha scritto: >>>>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> >>>>> --- >>>>> qidl.h | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 113 insertions(+) >>>>> create mode 100644 qidl.h >>>>> >>>>> diff --git a/qidl.h b/qidl.h >>>>> new file mode 100644 >>>>> index 0000000..eae0202 >>>>> --- /dev/null >>>>> +++ b/qidl.h >>>>> @@ -0,0 +1,113 @@ >>>>> +/* >>>>> + * QEMU IDL Macros/stubs >>>>> + * >>>>> + * See docs/qidl.txt for usage information. >>>>> + * >>>>> + * Copyright IBM, Corp. 2012 >>>>> + * >>>>> + * Authors: >>>>> + * Michael Roth <mdroth@linux.vnet.ibm.com> >>>>> + * >>>>> + * This work is licensed under the terms of the GNU GPLv2 or later. >>>>> + * See the COPYING file in the top-level directory. >>>>> + * >>>>> + */ >>>>> + >>>>> +#ifndef QIDL_H >>>>> +#define QIDL_H >>>>> + >>>>> +#include <glib.h> >>>>> +#include "qapi/qapi-visit-core.h" >>>>> +#include "qemu/object.h" >>>>> +#include "hw/qdev-properties.h" >>>>> + >>>>> +#ifdef QIDL_GEN >>>>> + >>>>> +/* we pass the code through the preprocessor with QIDL_GEN defined to parse >>>>> + * structures as they'd appear after preprocessing, and use the following >>>>> + * definitions mostly to re-insert the initial macros/annotations so they >>>>> + * stick around for the parser to process >>>>> + */ >>>>> +#define QIDL(...) QIDL(__VA_ARGS__) >>>>> +#define QIDL_START(name, ...) QIDL_START(name, ##__VA_ARGS__) >>>>> + >>>>> +#define QIDL_VISIT_TYPE(name, v, s, f, e) >>>>> +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) >>>>> +#define QIDL_PROPERTIES(name) >>>> >>>> Ok, a few questions... >>>> >>>> Why do you need these to expand to nothing in the QIDL_GEN case? >>>> >>> >>> They don't need to, I was just trying to be explicit about what >>> directives were relevant to the parser and which ones were relevant to >>> the actually compiled code. It was more a development "aid" than >>> anything else though, so I think we can drop the special handling and >>> clean these up a bit. >> >> Yes, thanks! >> >>>>> +#define QIDL_DECLARE(name, ...) \ >>>> >>>> Can QIDL_DECLARE replace QIDL_ENABLED as the magic detection string for >>>> qidl compilation? >>>> >>> >>> In some cases the declarations will come via #include'd headers, so the >>> only way to do that reliable is to run it through the preprocessor >>> first, which is how things were done in v1. But running everything >>> through cpp adds substantial overhead, and just because a QIDL-fied >>> struct is included in a C file, it doesn't mean that the C file intends >>> to use any qidl-generated code. >> >> Ok, I guess I need to see some example. We can clean it up later if we >> find a more clever way to do things. > > This was the main example I hit (not yet rebased): > > https://github.com/mdroth/qemu/commit/d8ea7c7a882e2fcbd0a9b7ab9ea47a389f87d31b > > As part of that patch We add annotations to PCIDevice in pci.h, which then gets > pulled in from quite a few devices. So we end up with *.qidl.c files for devices > that don't expose a "state" property or even have a QIDL_DECLARE() directive. > > If we were to scan for QIDL_DECLARE() in advance of running it through > the preprocessor, we'd address a lot of those case. But then we miss > cases like this: > > https://github.com/mdroth/qemu/commit/2199f721daebd5c3961069bdd51de80a5b4fa827 > > where, in pci.c, we use code generated from declarations in pci_internals.h even > though pci.c doesn't contain a QIDL_DECLARE() Hmm, this opens another can of worms. There is a substantial amount of duplicated code between generated files. For example, visit_type_PCIDevice is found in all *.qidl.c files for PCI devices. Worse, the same is true for the properties array. Right now, QIDL_DECLARE is a no-op at code-generation time. Could it be a marker to generate code for that particular struct? Then you would put a normal struct PCIDevice { }; declaration in hw/pci.h, and a QIDL_DECLARE(PCIDevice); in hw/pci.c that would trigger creation of the visitor etc. The code generator can also prepare extern declarations for types that are used but not defined, for example visit_type_PCIDevice in piix_pci.qidl.c. Paolo
On Mon, Oct 15, 2012 at 03:37:18PM +0200, Paolo Bonzini wrote: > Il 05/10/2012 18:47, Michael Roth ha scritto: > > On Fri, Oct 05, 2012 at 05:53:09PM +0200, Paolo Bonzini wrote: > >> Il 05/10/2012 17:41, Michael Roth ha scritto: > >>> On Fri, Oct 05, 2012 at 05:07:46PM +0200, Paolo Bonzini wrote: > >>>> Il 04/10/2012 19:33, Michael Roth ha scritto: > >>>>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > >>>>> --- > >>>>> qidl.h | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>>>> 1 file changed, 113 insertions(+) > >>>>> create mode 100644 qidl.h > >>>>> > >>>>> diff --git a/qidl.h b/qidl.h > >>>>> new file mode 100644 > >>>>> index 0000000..eae0202 > >>>>> --- /dev/null > >>>>> +++ b/qidl.h > >>>>> @@ -0,0 +1,113 @@ > >>>>> +/* > >>>>> + * QEMU IDL Macros/stubs > >>>>> + * > >>>>> + * See docs/qidl.txt for usage information. > >>>>> + * > >>>>> + * Copyright IBM, Corp. 2012 > >>>>> + * > >>>>> + * Authors: > >>>>> + * Michael Roth <mdroth@linux.vnet.ibm.com> > >>>>> + * > >>>>> + * This work is licensed under the terms of the GNU GPLv2 or later. > >>>>> + * See the COPYING file in the top-level directory. > >>>>> + * > >>>>> + */ > >>>>> + > >>>>> +#ifndef QIDL_H > >>>>> +#define QIDL_H > >>>>> + > >>>>> +#include <glib.h> > >>>>> +#include "qapi/qapi-visit-core.h" > >>>>> +#include "qemu/object.h" > >>>>> +#include "hw/qdev-properties.h" > >>>>> + > >>>>> +#ifdef QIDL_GEN > >>>>> + > >>>>> +/* we pass the code through the preprocessor with QIDL_GEN defined to parse > >>>>> + * structures as they'd appear after preprocessing, and use the following > >>>>> + * definitions mostly to re-insert the initial macros/annotations so they > >>>>> + * stick around for the parser to process > >>>>> + */ > >>>>> +#define QIDL(...) QIDL(__VA_ARGS__) > >>>>> +#define QIDL_START(name, ...) QIDL_START(name, ##__VA_ARGS__) > >>>>> + > >>>>> +#define QIDL_VISIT_TYPE(name, v, s, f, e) > >>>>> +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) > >>>>> +#define QIDL_PROPERTIES(name) > >>>> > >>>> Ok, a few questions... > >>>> > >>>> Why do you need these to expand to nothing in the QIDL_GEN case? > >>>> > >>> > >>> They don't need to, I was just trying to be explicit about what > >>> directives were relevant to the parser and which ones were relevant to > >>> the actually compiled code. It was more a development "aid" than > >>> anything else though, so I think we can drop the special handling and > >>> clean these up a bit. > >> > >> Yes, thanks! > >> > >>>>> +#define QIDL_DECLARE(name, ...) \ > >>>> > >>>> Can QIDL_DECLARE replace QIDL_ENABLED as the magic detection string for > >>>> qidl compilation? > >>>> > >>> > >>> In some cases the declarations will come via #include'd headers, so the > >>> only way to do that reliable is to run it through the preprocessor > >>> first, which is how things were done in v1. But running everything > >>> through cpp adds substantial overhead, and just because a QIDL-fied > >>> struct is included in a C file, it doesn't mean that the C file intends > >>> to use any qidl-generated code. > >> > >> Ok, I guess I need to see some example. We can clean it up later if we > >> find a more clever way to do things. > > > > This was the main example I hit (not yet rebased): > > > > https://github.com/mdroth/qemu/commit/d8ea7c7a882e2fcbd0a9b7ab9ea47a389f87d31b > > > > As part of that patch We add annotations to PCIDevice in pci.h, which then gets > > pulled in from quite a few devices. So we end up with *.qidl.c files for devices > > that don't expose a "state" property or even have a QIDL_DECLARE() directive. > > > > If we were to scan for QIDL_DECLARE() in advance of running it through > > the preprocessor, we'd address a lot of those case. But then we miss > > cases like this: > > > > https://github.com/mdroth/qemu/commit/2199f721daebd5c3961069bdd51de80a5b4fa827 > > > > where, in pci.c, we use code generated from declarations in pci_internals.h even > > though pci.c doesn't contain a QIDL_DECLARE() > > Hmm, this opens another can of worms. There is a substantial amount of > duplicated code between generated files. For example, > visit_type_PCIDevice is found in all *.qidl.c files for PCI devices. > Worse, the same is true for the properties array. > > Right now, QIDL_DECLARE is a no-op at code-generation time. Could it be > a marker to generate code for that particular struct? Then you would > put a normal > > struct PCIDevice { > }; > > declaration in hw/pci.h, and a > > QIDL_DECLARE(PCIDevice); > > in hw/pci.c that would trigger creation of the visitor etc. The code > generator can also prepare extern declarations for types that are used > but not defined, for example visit_type_PCIDevice in piix_pci.qidl.c. Hmm, this could work... what I'd probably do though is: - for cases where we're annotating "public" structs that may get pulled into multiple files, use QIDL_DECLARE_PUBLIC() in place of QIDL_DECLARE(). This will tell the code gen/qidl.h to only declare extern declarations for code we'll be linking against to access the generated visitors/properties/etc for that struct - within one or a few common objects that everyone links against (we can add one if one doesn't already exists), we can have, say, #include "hw/pci.h", and in the body we have a QIDL_IMPLEMENT_PUBLIC(PCIDevice), which tells the code gen to inject the code there like it would for QIDL_DECLARE(). If this seems reasonable, I could probably implement this as a follow-up, prior to any large-scale conversions. > > Paolo >
diff --git a/qidl.h b/qidl.h new file mode 100644 index 0000000..eae0202 --- /dev/null +++ b/qidl.h @@ -0,0 +1,113 @@ +/* + * QEMU IDL Macros/stubs + * + * See docs/qidl.txt for usage information. + * + * Copyright IBM, Corp. 2012 + * + * Authors: + * Michael Roth <mdroth@linux.vnet.ibm.com> + * + * This work is licensed under the terms of the GNU GPLv2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#ifndef QIDL_H +#define QIDL_H + +#include <glib.h> +#include "qapi/qapi-visit-core.h" +#include "qemu/object.h" +#include "hw/qdev-properties.h" + +#ifdef QIDL_GEN + +/* we pass the code through the preprocessor with QIDL_GEN defined to parse + * structures as they'd appear after preprocessing, and use the following + * definitions mostly to re-insert the initial macros/annotations so they + * stick around for the parser to process + */ +#define QIDL(...) QIDL(__VA_ARGS__) +#define QIDL_START(name, ...) QIDL_START(name, ##__VA_ARGS__) + +#define QIDL_VISIT_TYPE(name, v, s, f, e) +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) +#define QIDL_PROPERTIES(name) +#define QIDL_DECLARE(name, ...) \ + QIDL_START(name, ##__VA_ARGS__) \ + struct name + +#else /* !QIDL_GEN */ + +#define QIDL(...) +#ifdef QIDL_ENABLED +#define QIDL_START(name, ...) \ + static struct { \ + void (*visitor)(Visitor *, struct name **, const char *, Error **); \ + const char *schema_json_text; \ + Object *schema_obj; \ + Property *properties; \ + } qidl_data_##name; +#else +#define QIDL_START(name, ...) +#endif + +#define QIDL_DECLARE(name, ...) \ + QIDL_START(name, ##__VA_ARGS__) \ + struct name + +#define QIDL_VISIT_TYPE(name, v, s, f, e) \ + g_assert(qidl_data_##name.visitor); \ + qidl_data_##name.visitor(v, s, NULL, e) + +#define QIDL_SCHEMA_ADD_LINK(name, obj, path, errp) \ + g_assert(qidl_data_##name.schema_obj); \ + object_property_add_link(obj, path, "container", \ + &qidl_data_##name.schema_obj, errp) + +#define QIDL_PROPERTIES(name) \ + qidl_data_##name.properties + +#endif /* QIDL_GEN */ + +/* must be "called" in any C files that make use of QIDL-generated code */ +#define QIDL_ENABLE() + +/* QIDL annotations/markers + * + * q_immutable: state is fully restorable via device + * [re-]initialization/realization + * + * q_derived: state can be fully reconstructed from other fields (and will be, + * via [re-]initialization of the device or a separate hook) + * + * q_broken: state should (or possibly should) be saved, but isn't. mostly an aid + * for device developers having issues with serialization of a particular + * field, committed code should contain these except in special circumstances + * + * q_optional: <field> should only be serialized if the field by the name of + * has_<field> is true + * + * q_elsewhere: state should be serialized, but is done so elsewhere (for + * instance, by another device with a pointer to the same data) + * + * q_size(field): for static/dynamically-allocated arrays. specifies the field + * in the structure containing the number of elements that should be + * serialized. if argument is wrapped in parenthesis it is instead interpreted + * as an expression that should be invaluated to determine the size. + * + * q_property(<property name> [, <default value>]): specifies that field is a + * qdev-style property. all properties of the struct are then accessible via + * QIDL_PROPERTIES(<device name>) macro. + */ + +#define q_immutable QIDL(immutable) +#define q_derived QIDL(derived) +#define q_broken QIDL(broken) +#define q_optional QIDL(optional) +#define q_elsewhere QIDL(elsewhere) +#define q_size(...) QIDL(size_is, ##__VA_ARGS__) +#define q_property(name, ...) QIDL(property, name, ##__VA_ARGS__) + +#endif
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- qidl.h | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 qidl.h