Patchwork [v4,24/26] qidl: add QAPI-based code generator

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 16, 2012, 7:20 a.m.
Message ID <507D0AA9.6050204@redhat.com>
Download mbox | patch
Permalink /patch/191741/
State New
Headers show

Comments

Paolo Bonzini - Oct. 16, 2012, 7:20 a.m.
Il 15/10/2012 18:35, Michael Roth ha scritto:
>> - immutable/derived/broken/elsewhere (and the default, let's call it
>> serialized) are really five cases of the same QIDL property.  Perhaps
>> this could be enforced in the extended syntax like this:
>>
>>     #define q_immutable QIDL(serialize(immutable))
>>     #define q_derived QIDL(serialize(derived))
>>     #define q_broken QIDL(serialize(broken))
>>     #define q_elsewhere QIDL(serialize(elsewhere))
>>
>> I would also make it possible to explicitly specify the fifth state, if
>> only for symmetry.
> 
> Agreed, that's a more proper grouping. Though, for consistency with
> QIDL(property, ...), I would do QIDL(serialize, ...)

Sure.

>> - it would be _much_ better if you could automatically derive properties
>> information for embedded structs.  For example, Notifiers and qemu_irqs
>> are always q_immutable.  NotifierLists probably are always q_elsewhere,
>> because the owner of the notifiers should add themselves back.
> 
> q_inherit maybe? Otherwise we're overriding "q_default" in subtle
> ways that may not always be desired.

I think the default should be "whatever makes more sense", which for
QIDL_DECLAREd types means making the member q_immutable if it makes
sense for the type.

It's too fragile to expect all subclasses to know whether their
superclass is immutable or has to be serialized, so q_inherit should be
the default.  For atomic types, q_inherit is the same as q_serialized.

That said, an alternative is just to never declare the superclass
q_immutable.  That would work as long as you do not restrict QIDL to
DeviceState subclasses---see attached patch.

>> In general, if struct X is QIDL_DECLAREd and only has q_immutable
>> fields, it can be taken as q_immutable.  Hence for example the base
>> class should not need any decoration; ISADevice will be seen as
>> q_immutable, but PCIDevice will be seen as serialized.  But even if a
>> struct is not QIDL_DECLAREd, it  should be possible to apply a tag to a
>> typedef, and have it always applied to the members.

Hmm, this wasn't the best choice of words.  What I actually meant was
"to apply a tag to a typedef, and have it always applied to members of
that type in other structs".  Like

typedef struct Notifier Notifier q_immutable;

Note that Notifier will never have any serializable state, hence it will
not be QIDL_DECLAREd.  It is just a proxy that specifies a function to call.

While in principle it is possible to change the function at run-time,
that's not the way that Notifiers are used.  That can still be
documented using q_elsewhere, but I think that sane defaults other than
q_serialized are useful to avoid cluttering the declarations.  However,
this is a very minor qualm.

Paolo
Michael Roth - Oct. 19, 2012, 3:06 a.m.
On Tue, Oct 16, 2012 at 09:20:09AM +0200, Paolo Bonzini wrote:
> Il 15/10/2012 18:35, Michael Roth ha scritto:
> >> - immutable/derived/broken/elsewhere (and the default, let's call it
> >> serialized) are really five cases of the same QIDL property.  Perhaps
> >> this could be enforced in the extended syntax like this:
> >>
> >>     #define q_immutable QIDL(serialize(immutable))
> >>     #define q_derived QIDL(serialize(derived))
> >>     #define q_broken QIDL(serialize(broken))
> >>     #define q_elsewhere QIDL(serialize(elsewhere))
> >>
> >> I would also make it possible to explicitly specify the fifth state, if
> >> only for symmetry.
> > 
> > Agreed, that's a more proper grouping. Though, for consistency with
> > QIDL(property, ...), I would do QIDL(serialize, ...)
> 
> Sure.
> 
> >> - it would be _much_ better if you could automatically derive properties
> >> information for embedded structs.  For example, Notifiers and qemu_irqs
> >> are always q_immutable.  NotifierLists probably are always q_elsewhere,
> >> because the owner of the notifiers should add themselves back.
> > 
> > q_inherit maybe? Otherwise we're overriding "q_default" in subtle
> > ways that may not always be desired.
> 
> I think the default should be "whatever makes more sense", which for
> QIDL_DECLAREd types means making the member q_immutable if it makes
> sense for the type.
> 
> It's too fragile to expect all subclasses to know whether their
> superclass is immutable or has to be serialized, so q_inherit should be
> the default.  For atomic types, q_inherit is the same as q_serialized.

Totally agree. And over time, as more and more device-related structures are
QIDL_DECLAREd, this will help a lot on cutting down "boilerplate"
annotations.

> 
> That said, an alternative is just to never declare the superclass
> q_immutable.  That would work as long as you do not restrict QIDL to
> DeviceState subclasses---see attached patch.

Yah, I think the best general policy is to make as many of the fields
Just Work as possible by default. This can be done by using QIDL_DECLARE()
for as many relevant struct fields as possible (and the ones that aren't
QIDL_DECLAREd (or have an open-coded visitor) will generate errors that
can prompt further action (the optimal/encouraged one being to QIDL_DECLARE
the struct in question, or creating an open-coded visitor for the
simpler cases.

This how I'd prefer we handle making inherit-by-default work better, and
I'll focus on this more in the conversions.

> 
> >> In general, if struct X is QIDL_DECLAREd and only has q_immutable
> >> fields, it can be taken as q_immutable.  Hence for example the base
> >> class should not need any decoration; ISADevice will be seen as
> >> q_immutable, but PCIDevice will be seen as serialized.  But even if a
> >> struct is not QIDL_DECLAREd, it  should be possible to apply a tag to a
> >> typedef, and have it always applied to the members.
> 
> Hmm, this wasn't the best choice of words.  What I actually meant was
> "to apply a tag to a typedef, and have it always applied to members of
> that type in other structs".  Like
> 
> typedef struct Notifier Notifier q_immutable;

This would be really nice, but the parser is gonna need more hardening
before we can remove it's "scan for lines beginning with QIDL_START* and
parse what we expect to come afterward" behavior. There's also
build time implications. An alternative I'm considering is:

typedef struct Notifier Notifier;

QIDL_DECLARE_PUBLIC(Notifier, q_immutable) {
    ...
}

This will make all the fields q_immutable by default (individual fields
can override it in the future when the need arises). Then we teach the
code generator to drop serialization for any struct fields who don't
have any serializeable fields (mainly to avoid a bunch of nil fields in
the serialized code (e.g. 'notifier1': {})).

I'm planning on doing the above in the context of the device conversions
to get them a little more palatable.

Seem reasonable?

> 
> Note that Notifier will never have any serializable state, hence it will
> not be QIDL_DECLAREd.  It is just a proxy that specifies a function to call.
> 
> While in principle it is possible to change the function at run-time,
> that's not the way that Notifiers are used.  That can still be
> documented using q_elsewhere, but I think that sane defaults other than
> q_serialized are useful to avoid cluttering the declarations.  However,
> this is a very minor qualm.
> 
> Paolo
Paolo Bonzini - Oct. 19, 2012, 9:01 a.m.
> > >> In general, if struct X is QIDL_DECLAREd and only has
> > >> q_immutable
> > >> fields, it can be taken as q_immutable.  Hence for example the
> > >> base
> > >> class should not need any decoration; ISADevice will be seen as
> > >> q_immutable, but PCIDevice will be seen as serialized.  But even
> > >> if a
> > >> struct is not QIDL_DECLAREd, it  should be possible to apply a
> > >> tag to a
> > >> typedef, and have it always applied to the members.
> > 
> > Hmm, this wasn't the best choice of words.  What I actually meant
> > was "to apply a tag to a typedef, and have it always applied to members
> > of that type in other structs".  Like
> > 
> > typedef struct Notifier Notifier q_immutable;
> 
> This would be really nice, but the parser is gonna need more hardening
> before we can remove it's "scan for lines beginning with QIDL_START*
> and parse what we expect to come afterward" behavior. There's also
> build time implications. An alternative I'm considering is:
> 
> typedef struct Notifier Notifier;
> 
> QIDL_DECLARE_PUBLIC(Notifier, q_immutable) {
>     ...
> }
> 
> This will make all the fields q_immutable by default (individual fields
> can override it in the future when the need arises). Then we teach the
> code generator to drop serialization for any struct fields who don't
> have any serializeable fields (mainly to avoid a bunch of nil fields
> in the serialized code (e.g. 'notifier1': {})).
> 
> Seem reasonable?

Optimal, no... reasonable, yes. :)

Paolo

Patch

From 0629eab1d8cff0764d135b85052dd3ba47a1a198 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 16 Oct 2012 09:19:04 +0200
Subject: [PATCH] qidl: allow marking Object as QIDL_DECLARE

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/isa.h              |  8 ++++----
 hw/mc146818rtc.c      |  2 +-
 hw/qdev.h             | 28 ++++++++++++++--------------
 include/qemu/object.h | 11 ++++++-----
 qidl.h                |  6 ++----
 scripts/qidl.py       |  2 +-
 6 file modificati, 28 inserzioni(+), 29 rimozioni(-)

diff --git a/hw/isa.h b/hw/isa.h
index 8fb498a..6c7d815 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -31,11 +31,11 @@  struct ISABus {
     qemu_irq *irqs;
 };
 
-struct ISADevice {
+QIDL_DECLARE(ISADevice) {
     DeviceState qdev;
-    uint32_t isairq[2];
-    int nirqs;
-    int ioport_id;
+    uint32_t q_immutable isairq[2];
+    int q_immutable nirqs;
+    int q_immutable ioport_id;
 };
 
 ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space_io);
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index f5f4bb4..b29cce5 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -62,7 +62,7 @@  QIDL_ENABLE()
 typedef struct RTCState RTCState;
 
 QIDL_DECLARE(RTCState) {
-    ISADevice q_immutable dev;
+    ISADevice dev;
     MemoryRegion q_immutable io;
     uint8_t cmos_data[128];
     uint8_t cmos_index;
diff --git a/hw/qdev.h b/hw/qdev.h
index 6f2c7f2..3dba377 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -57,22 +57,22 @@  typedef struct DeviceClass {
 
 /* This structure should not be accessed directly.  We declare it here
    so that it can be embedded in individual device state structures.  */
-struct DeviceState {
+QIDL_DECLARE(DeviceState) {
     Object parent_obj;
 
-    const char *id;
-    enum DevState state;
-    QemuOpts *opts;
-    int hotplugged;
-    BusState *parent_bus;
-    int num_gpio_out;
-    qemu_irq *gpio_out;
-    int num_gpio_in;
-    qemu_irq *gpio_in;
-    QLIST_HEAD(, BusState) child_bus;
-    int num_child_bus;
-    int instance_id_alias;
-    int alias_required_for_version;
+    const char q_immutable *id;
+    enum DevState q_immutable state;
+    QemuOpts q_immutable *opts;
+    int q_immutable hotplugged;
+    BusState q_immutable *parent_bus;
+    int q_immutable num_gpio_out;
+    qemu_irq q_immutable *gpio_out;
+    int q_immutable num_gpio_in;
+    qemu_irq q_immutable *gpio_in;
+    QLIST_HEAD(, BusState) q_immutable child_bus;
+    int q_immutable num_child_bus;
+    int q_immutable instance_id_alias;
+    int q_immutable alias_required_for_version;
 };
 
 #define TYPE_BUS "bus"
diff --git a/include/qemu/object.h b/include/qemu/object.h
index cc75fee..cb4a89b 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -18,6 +18,7 @@ 
 #include <stdint.h>
 #include <stdbool.h>
 #include "qemu-queue.h"
+#include "qidl.h"
 
 struct Visitor;
 struct Error;
@@ -257,13 +258,13 @@  struct ObjectClass
  * #Object also contains a list of #Interfaces that this object
  * implements.
  */
-struct Object
+QIDL_DECLARE(Object)
 {
     /*< private >*/
-    ObjectClass *class;
-    QTAILQ_HEAD(, ObjectProperty) properties;
-    uint32_t ref;
-    Object *parent;
+    ObjectClass q_immutable *class;
+    QTAILQ_HEAD(, ObjectProperty) q_immutable properties;
+    uint32_t q_elsewhere ref;
+    Object q_immutable *parent;
 };
 
 /**
diff --git a/qidl.h b/qidl.h
index b4c9b02..abff6ba 100644
--- a/qidl.h
+++ b/qidl.h
@@ -18,8 +18,6 @@ 
 
 #include <glib.h>
 #include "qapi/qapi-visit-core.h"
-#include "qemu/object.h"
-#include "hw/qdev-properties.h"
 
 /* must be "called" in any C files that make use of QIDL-generated code */
 #define QIDL_ENABLE()
@@ -42,8 +40,8 @@ 
     static struct { \
         void (*visitor)(Visitor *, struct name **, const char *, Error **); \
         const char *schema_json_text; \
-        Object *schema_obj; \
-        Property *properties; \
+        struct Object *schema_obj; \
+        struct Property *properties; \
     } qidl_data_##name;
 #else
 #define QIDL_START(name,  ...)
diff --git a/scripts/qidl.py b/scripts/qidl.py
index 4e0880e..2544829 100644
--- a/scripts/qidl.py
+++ b/scripts/qidl.py
@@ -240,7 +240,7 @@  def main(argv=[]):
 
     output_filepath = None
     schema_filepath = None
-    includes = []
+    includes = ["hw/qdev-properties.h"]
     for o, a in opts:
         if o in ("-f", "--output-filepath"):
             output_filepath = a
-- 
1.7.12.1