Patchwork [v3,21/22] qidl: qidl.h, definitions for qidl annotations

login
register
mail settings
Submitter Michael Roth
Date Oct. 4, 2012, 5:33 p.m.
Message ID <1349372021-31212-22-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/189221/
State New
Headers show

Comments

Michael Roth - Oct. 4, 2012, 5:33 p.m.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qidl.h |  113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100644 qidl.h
Paolo Bonzini - Oct. 5, 2012, 8:14 a.m.
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)
> +
Michael Roth - Oct. 5, 2012, 2:50 p.m.
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)
> > +
> 
>
Paolo Bonzini - Oct. 5, 2012, 3:07 p.m.
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
>
Michael Roth - Oct. 5, 2012, 3:41 p.m.
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
> > 
> 
>
Paolo Bonzini - Oct. 5, 2012, 3:53 p.m.
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
Michael Roth - Oct. 5, 2012, 4:47 p.m.
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
>
Paolo Bonzini - Oct. 15, 2012, 1:37 p.m.
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
Michael Roth - Oct. 15, 2012, 3:50 p.m.
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
>

Patch

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