Re: [Qemu-devel] [PATCH v2 01/10] qapi: add Visitor interfaces for uint*_t and int*_t

[Paolo Bonzini <pbonzini@redhat.com>]

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

On 12/20/2011 12:12 PM, Paolo Bonzini wrote:
>
> I think this approach is wrong.  We're mashing the design of vmstate
> with that of visitors and getting something that is not a visitor and
> not vmstate.
>
> Instead, I think you should have something like this:
>
>      struct VMStateInfo {
>          const char *name;
>          // takes a QMPOutputVisitor and a QEMUFile open for reading
>          int (*load)(QEMUFile *f, const char *name, Visitor *v,
>                      size_t size, Error **err);
>
>          // takes a QMPInputVisitor and a QEMUFile open for writing
>          void (*save)(QEMUFile *f, const char *name, Visitor *v,
>                      size_t size, Error **err);
>
>          // takes a QMPOutputVisitor and reads from *pv
>          int (*get)(Visitor *v, const char *name, void *pv,
>                     size_t size, Error **err);
>
>          // takes a QMPInputVisitor and writes into *pv
>          void (*set)(Visitor *v, const char *name, void *pv,
>                     size_t size, Error **err);
>      };
>
> that splits the existing callbacks in two steps.
>
> For saving, you would adapt your visitor-based vmstate "put" routines so
> that they put things in a dictionary with no regard for integer types (a
> bit ugly for uint64, but perfectly fine for everything else).  You take
> the dictionary from the output visitor and (with an input visitor) you
> feed it back to the "save" routines, which convert the dictionary to a
> QEMUFile.  Both steps keep the types internal to vmstate.
>
> For loading, it's the other way round: you interpret the vmstate with
> the QEMUFile reading routines, and create a dictionary.  Then make an
> input visitor and use the vmstate "set" interpreter to fill in the
> struct fields.
>
> I'm sorry for noticing this just now, I was waiting for Anthony's QOM
> plans to be committed so that I could understand better how vmstate and
> QOM properties would interact.  In fact, it would be great and not hard
> if the struct<->visitor step (get/set) was also exposed as a QOM property.

Note that this doesn't prevent sharing the code for loading and saving.

1) You can still add a vtable to QEMUFile for "visit_type_int*" and 
"visit_type_uint*".  But this vtable doesn't need start/end callbacks.

2) Similarly, I don't object to adding visit_type_int* and 
visit_type_uint* to Visitor.  However, these should be exclusively a 
convenience for the callers, so that they do not have to convert between 
int64 and other types.  There should be exactly two implementations of 
these callbacks, one for input visitors (including e.g. the dealloc 
visitor) and one for output visitors.  I attach a sample patch that does 
this for int16 only.

Paolo

[-- Attachment #2: qapi-visitor-types.patch --]
[-- Type: text/x-patch, Size: 4682 bytes --]

diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index a154523..17964ad 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -154,6 +154,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
 
     v = g_malloc0(sizeof(*v));
 
+    qapi_init_input_visitor(&v->visitor);
     v->visitor.start_struct = qapi_dealloc_start_struct;
     v->visitor.end_struct = qapi_dealloc_end_struct;
     v->visitor.start_list = qapi_dealloc_start_list;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index ddef3ed..5c1881d 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -116,3 +116,37 @@ void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
         v->type_number(v, obj, name, errp);
     }
 }
+
+static void visit_type_int16_in(Visitor *v, int16_t *obj, const char *name, Error **errp)
+{
+    if (!error_is_set(errp)) {
+        int64_t value;
+        v->type_int(v, &value, name, errp);
+        *obj = value;
+    }
+}
+
+static void visit_type_int16_out(Visitor *v, int16_t *obj, const char *name, Error **errp)
+{
+    if (!error_is_set(errp)) {
+        int64_t value = *obj;
+        v->type_int(v, &value, name, errp);
+    }
+}
+
+void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp)
+{
+    if (!error_is_set(errp)) {
+        v->type_int16(v, obj, name, errp);
+    }
+}
+
+void qapi_init_input_visitor(Visitor *v)
+{
+    v->type_int16 = visit_type_int16_in;
+}
+
+void qapi_init_output_visitor(Visitor *v)
+{
+    v->type_int16 = visit_type_int16_out;
+}
diff --git a/qapi/qapi-visit-core.h b/qapi/qapi-visit-core.h
index e850746..c0395b3 100644
--- a/qapi/qapi-visit-core.h
+++ b/qapi/qapi-visit-core.h
@@ -52,6 +52,9 @@ struct Visitor
     void (*start_handle)(Visitor *v, void **obj, const char *kind,
                          const char *name, Error **errp);
     void (*end_handle)(Visitor *v, Error **errp);
+
+    /* Internal only.  */
+    void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp);
 };
 
 void visit_start_handle(Visitor *v, void **obj, const char *kind,
@@ -69,8 +72,24 @@ void visit_end_optional(Visitor *v, Error **errp);
 void visit_type_enum(Visitor *v, int *obj, const char *strings[],
                      const char *kind, const char *name, Error **errp);
 void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp);
+void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp);
 void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
 void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
 void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
 
+static inline void visit_type_uint(Visitor *v, uint64_t *obj,
+                                   const char *name, Error **errp)
+{
+    visit_type_int(v, (int64_t *) obj, name, errp);
+}
+
+static inline void visit_type_uint16(Visitor *v, uint16_t *obj,
+                                     const char *name, Error **errp)
+{
+    visit_type_int16(v, (int16_t *) obj, name, errp);
+}
+
+void qapi_init_input_visitor(Visitor *v);
+void qapi_init_output_visitor(Visitor *v);
+
 #endif
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index c78022b..67da359 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -283,6 +283,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
 
     v = g_malloc0(sizeof(*v));
 
+    qapi_init_input_visitor(&v->visitor);
     v->visitor.start_struct = qmp_input_start_struct;
     v->visitor.end_struct = qmp_input_end_struct;
     v->visitor.start_list = qmp_input_start_list;
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index f76d015..65f8e3e 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -234,6 +234,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
 
     v = g_malloc0(sizeof(*v));
 
+    qapi_init_output_visitor(&v->visitor);
     v->visitor.start_struct = qmp_output_start_struct;
     v->visitor.end_struct = qmp_output_end_struct;
     v->visitor.start_list = qmp_output_start_list;
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 663c2a0..77bc529 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -229,10 +229,8 @@ static void get_int16(DeviceState *dev, Visitor *v, void *opaque,
 {
     Property *prop = opaque;
     int16_t *ptr = qdev_get_prop_ptr(dev, prop);
-    int64_t value;
 
-    value = *ptr;
-    visit_type_int(v, &value, name, errp);
+    visit_type_int16(v, ptr, name, errp);
 }
 
 static void set_int16(DeviceState *dev, Visitor *v, void *opaque,