Re: [Qemu-devel] [PATCH v4 24/26] qidl: add QAPI-based code generator

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2744 bytes --]

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

[-- Attachment #2: 0001-qidl-allow-marking-Object-as-QIDL_DECLARE.patch --]
[-- Type: text/x-patch, Size: 4444 bytes --]

>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