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
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,
