[Qemu-devel] [PATCH 0/2] qom-get [for 2.8]

[Qemu-devel] [PATCH 0/2] qom-get [for 2.8]

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  I'd noticed a while ago the lack of qom-get and
was about to implement it, when I noticed Andreas
had implemented it back in March 2015 as part of the
larger patch series 'qom: HMP commands to supersede info qtree'
( 1426178624-32638-1-git-send-email-afaerber@suse.de )
which was a v2 of a series from 2014 of his.

Here is a forward ported version of just the pair of
patches that are needed for qom-get.

Usage examples:

(qemu) qom-get /backend/console[0]/device/vga.rom[0] size
65536 (0x10000)

(qemu) qom-get /machine smm
"auto"

(qemu) qom-get /machine rtc-time
struct type not implemented

Dave

Dr. David Alan Gilbert (2):
  qapi: Stub out StringOutputVisitor struct support
  qom: Implement qom-get HMP command

 hmp-commands.hx              | 13 +++++++++++++
 hmp.c                        | 23 +++++++++++++++++++++++
 hmp.h                        |  1 +
 qapi/string-output-visitor.c | 13 +++++++++++++
 4 files changed, 50 insertions(+)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Avoid a segfault when visiting, e.g., the QOM rtc-time property,
by implementing the struct callbacks and raising an Error.

Signed-off-by: Andreas Färber <afaerber@suse.de>

Updated for changed interface:
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 qapi/string-output-visitor.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 94ac821..4e7e97f 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
+#include "qapi/error.h"
 #include "qapi/string-output-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qemu/host-utils.h"
@@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
     string_output_set(sov, g_strdup_printf("%f", *obj));
 }
 
+static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
+           Error **errp)
+{
+    error_setg(errp, "struct type not implemented");
+}
+
+static void end_struct(Visitor *v, void **obj)
+{
+}
+
 static void
 start_list(Visitor *v, const char *name, GenericList **list, size_t size,
            Error **errp)
@@ -356,6 +367,8 @@ Visitor *string_output_visitor_new(bool human, char **result)
     v->visitor.end_list = end_list;
     v->visitor.complete = string_output_complete;
     v->visitor.free = string_output_free;
+    v->visitor.start_struct = start_struct;
+    v->visitor.end_struct = end_struct;
 
     return &v->visitor;
 }
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] qom: Implement qom-get HMP command

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Reimplement it based on qmp_qom_get() to avoid converting QObjects back
to strings.

Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>

Slight fix for bit-rot:
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp-commands.hx | 13 +++++++++++++
 hmp.c           | 23 +++++++++++++++++++++++
 hmp.h           |  1 +
 3 files changed, 37 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 848efee..73f0372 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1736,6 +1736,19 @@ Print QOM properties of object at location @var{path}
 ETEXI
 
     {
+        .name       = "qom-get",
+        .args_type  = "path:s,property:s",
+        .params     = "path property",
+        .help       = "print QOM property",
+        .mhandler.cmd  = hmp_qom_get,
+    },
+
+STEXI
+@item qom-get @var{path} @var{property}
+Print QOM property @var{property} of object at location @var{path}
+ETEXI
+
+    {
         .name       = "qom-set",
         .args_type  = "path:s,property:s,value:s",
         .params     = "path property value",
diff --git a/hmp.c b/hmp.c
index cc2056e..17e6b06 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2064,6 +2064,29 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+void hmp_qom_get(Monitor *mon, const QDict *qdict)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const char *property = qdict_get_str(qdict, "property");
+    Error *err = NULL;
+    Object *obj;
+    char *value;
+
+    obj = object_resolve_path(path, NULL);
+    if (obj == NULL) {
+        error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Device '%s' not found", path);
+        hmp_handle_error(mon, &err);
+        return;
+    }
+    value = object_property_print(obj, property, true, &err);
+    if (err == NULL) {
+        monitor_printf(mon, "%s\n", value);
+        g_free(value);
+    }
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_qom_set(Monitor *mon, const QDict *qdict)
 {
     const char *path = qdict_get_str(qdict, "path");
diff --git a/hmp.h b/hmp.h
index 0876ec0..882f339 100644
--- a/hmp.h
+++ b/hmp.h
@@ -103,6 +103,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict);
 void hmp_info_memdev(Monitor *mon, const QDict *qdict);
 void hmp_info_memory_devices(Monitor *mon, const QDict *qdict);
 void hmp_qom_list(Monitor *mon, const QDict *qdict);
+void hmp_qom_get(Monitor *mon, const QDict *qdict);
 void hmp_qom_set(Monitor *mon, const QDict *qdict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/2] qom-get [for 2.8]

[Paolo Bonzini]

On 25/08/2016 11:37, Dr. David Alan Gilbert (git) wrote:
> 
> (qemu) qom-get /backend/console[0]/device/vga.rom[0] size
> 65536 (0x10000)
> 
> (qemu) qom-get /machine smm
> "auto"
> 
> (qemu) qom-get /machine rtc-time
> struct type not implemented

Looks nice.  Perhaps instead of using the "human" interface we could
just print it as JSON and avoid the error.

Paolo

Re: [Qemu-devel] [PATCH 0/2] qom-get [for 2.8]

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 25/08/2016 11:37, Dr. David Alan Gilbert (git) wrote:
> > 
> > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size
> > 65536 (0x10000)
> > 
> > (qemu) qom-get /machine smm
> > "auto"
> > 
> > (qemu) qom-get /machine rtc-time
> > struct type not implemented
> 
> Looks nice.  Perhaps instead of using the "human" interface we could
> just print it as JSON and avoid the error.

I can't see a sane place to glue a fallback to the JSON code in;
or do you mean use the JSON output even in the simple case?

What's really needed is for the string-output-visitor to be forcibly
reworked for proper nesting, but that's a bit of work.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/2] qom-get [for 2.8]

[Paolo Bonzini]

On 05/09/2016 20:50, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>
>>
>> On 25/08/2016 11:37, Dr. David Alan Gilbert (git) wrote:
>>>
>>> (qemu) qom-get /backend/console[0]/device/vga.rom[0] size
>>> 65536 (0x10000)
>>>
>>> (qemu) qom-get /machine smm
>>> "auto"
>>>
>>> (qemu) qom-get /machine rtc-time
>>> struct type not implemented
>>
>> Looks nice.  Perhaps instead of using the "human" interface we could
>> just print it as JSON and avoid the error.
> 
> I can't see a sane place to glue a fallback to the JSON code in;
> or do you mean use the JSON output even in the simple case?

Yeah.  I think it makes sense for a low level command such as qom-get.
All that you lose is the automatic hex conversion in the first example.

Paolo

Re: [Qemu-devel] [PATCH 0/2] qom-get [for 2.8]

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 05/09/2016 20:50, Dr. David Alan Gilbert wrote:
> > * Paolo Bonzini (pbonzini@redhat.com) wrote:
> >>
> >>
> >> On 25/08/2016 11:37, Dr. David Alan Gilbert (git) wrote:
> >>>
> >>> (qemu) qom-get /backend/console[0]/device/vga.rom[0] size
> >>> 65536 (0x10000)
> >>>
> >>> (qemu) qom-get /machine smm
> >>> "auto"
> >>>
> >>> (qemu) qom-get /machine rtc-time
> >>> struct type not implemented
> >>
> >> Looks nice.  Perhaps instead of using the "human" interface we could
> >> just print it as JSON and avoid the error.
> > 
> > I can't see a sane place to glue a fallback to the JSON code in;
> > or do you mean use the JSON output even in the simple case?
> 
> Yeah.  I think it makes sense for a low level command such as qom-get.
> All that you lose is the automatic hex conversion in the first example.

Hmm yes, it does - v2 coming up.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Avoid a segfault when visiting, e.g., the QOM rtc-time property,
> by implementing the struct callbacks and raising an Error.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
>
> Updated for changed interface:
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  qapi/string-output-visitor.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 94ac821..4e7e97f 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -12,6 +12,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> +#include "qapi/error.h"
>  #include "qapi/string-output-visitor.h"
>  #include "qapi/visitor-impl.h"
>  #include "qemu/host-utils.h"
> @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
>      string_output_set(sov, g_strdup_printf("%f", *obj));
>  }
>  
> +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
> +           Error **errp)
> +{
> +    error_setg(errp, "struct type not implemented");
> +}
> +
> +static void end_struct(Visitor *v, void **obj)
> +{
> +}
> +

This is just one of the several things this visitor doesn't implement.
See the comment in string-output-visitor.h.

String input visitor and options visitor have similar holes; see the
comments in string-input-visitor.h and opts-visitor.h.

Should we change all of them together to report errors instead of crash?
With common "error out because this isn't implemented" methods?

>  static void
>  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
>             Error **errp)
> @@ -356,6 +367,8 @@ Visitor *string_output_visitor_new(bool human, char **result)
>      v->visitor.end_list = end_list;
>      v->visitor.complete = string_output_complete;
>      v->visitor.free = string_output_free;
> +    v->visitor.start_struct = start_struct;
> +    v->visitor.end_struct = end_struct;
>  
>      return &v->visitor;
>  }

Re: [Qemu-devel] [PATCH 2/2] qom: Implement qom-get HMP command

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Reimplement it based on qmp_qom_get() to avoid converting QObjects back
> to strings.

It's not using qmp_qom_get(), though.  That's a sign our abstractions
aren't quite right.  HMP command handlers should be implemented as
mostly trivial wrappers around the QMP handlers, or the two handlers
should be mostly trivial wrappers around a common helper function.  For
a more detailed explanation, see below.

Hmm, existing hmp_qom_set() has the same issue.  I'm not demanding you
clean this up.

> Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
>
> Slight fix for bit-rot:
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hmp-commands.hx | 13 +++++++++++++
>  hmp.c           | 23 +++++++++++++++++++++++
>  hmp.h           |  1 +
>  3 files changed, 37 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 848efee..73f0372 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1736,6 +1736,19 @@ Print QOM properties of object at location @var{path}
>  ETEXI
>  
>      {
> +        .name       = "qom-get",
> +        .args_type  = "path:s,property:s",
> +        .params     = "path property",
> +        .help       = "print QOM property",
> +        .mhandler.cmd  = hmp_qom_get,
> +    },
> +
> +STEXI
> +@item qom-get @var{path} @var{property}
> +Print QOM property @var{property} of object at location @var{path}

This is a bit terse, but existing qom-list and qom-set are no less
terse.  The QAPI schema has more detail, perhaps we can adapt it for the
manual.  Not a blocker for me.

> +ETEXI
> +
> +    {
>          .name       = "qom-set",
>          .args_type  = "path:s,property:s,value:s",
>          .params     = "path property value",
> diff --git a/hmp.c b/hmp.c
> index cc2056e..17e6b06 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2064,6 +2064,29 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +void hmp_qom_get(Monitor *mon, const QDict *qdict)
> +{
> +    const char *path = qdict_get_str(qdict, "path");
> +    const char *property = qdict_get_str(qdict, "property");
> +    Error *err = NULL;
> +    Object *obj;
> +    char *value;
> +
> +    obj = object_resolve_path(path, NULL);
> +    if (obj == NULL) {
> +        error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                  "Device '%s' not found", path);
> +        hmp_handle_error(mon, &err);
> +        return;
> +    }
> +    value = object_property_print(obj, property, true, &err);
> +    if (err == NULL) {
> +        monitor_printf(mon, "%s\n", value);
> +        g_free(value);
> +    }
> +    hmp_handle_error(mon, &err);
> +}
> +

Compare:

   QObject *qmp_qom_get(const char *path, const char *property, Error **errp)
   {
       Object *obj;

       obj = object_resolve_path(path, NULL);
       if (!obj) {
           error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                     "Device '%s' not found", path);
           return NULL;
       }

       return object_property_get_qobject(obj, property, errp);
   }

The missing piece seems to be a function that prints the result of
object_property_get_qobject().  Since converting to QObject just for
printing is stupid, a common helper function seems more appropriate.
qmp_qom_get() would pass it the QMP output visitor to get a QObject, and
hmp_qom_get() would pass it the string output visitor to get a string.

>  void hmp_qom_set(Monitor *mon, const QDict *qdict)
>  {
>      const char *path = qdict_get_str(qdict, "path");
> diff --git a/hmp.h b/hmp.h
> index 0876ec0..882f339 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -103,6 +103,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict);
>  void hmp_info_memdev(Monitor *mon, const QDict *qdict);
>  void hmp_info_memory_devices(Monitor *mon, const QDict *qdict);
>  void hmp_qom_list(Monitor *mon, const QDict *qdict);
> +void hmp_qom_get(Monitor *mon, const QDict *qdict);
>  void hmp_qom_set(Monitor *mon, const QDict *qdict);
>  void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
>  void object_del_completion(ReadLineState *rs, int nb_args, const char *str);

Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Avoid a segfault when visiting, e.g., the QOM rtc-time property,
> > by implementing the struct callbacks and raising an Error.
> >
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> >
> > Updated for changed interface:
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  qapi/string-output-visitor.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> > index 94ac821..4e7e97f 100644
> > --- a/qapi/string-output-visitor.c
> > +++ b/qapi/string-output-visitor.c
> > @@ -12,6 +12,7 @@
> >  
> >  #include "qemu/osdep.h"
> >  #include "qemu-common.h"
> > +#include "qapi/error.h"
> >  #include "qapi/string-output-visitor.h"
> >  #include "qapi/visitor-impl.h"
> >  #include "qemu/host-utils.h"
> > @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
> >      string_output_set(sov, g_strdup_printf("%f", *obj));
> >  }
> >  
> > +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
> > +           Error **errp)
> > +{
> > +    error_setg(errp, "struct type not implemented");
> > +}
> > +
> > +static void end_struct(Visitor *v, void **obj)
> > +{
> > +}
> > +
> 
> This is just one of the several things this visitor doesn't implement.
> See the comment in string-output-visitor.h.
> 
> String input visitor and options visitor have similar holes; see the
> comments in string-input-visitor.h and opts-visitor.h.
> 
> Should we change all of them together to report errors instead of crash?
> With common "error out because this isn't implemented" methods?

In that case wouldn't it be best to change visit_start_struct/visit_end_struct
to do the check (Like visit_check_struct does).

Dave

> 
> >  static void
> >  start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> >             Error **errp)
> > @@ -356,6 +367,8 @@ Visitor *string_output_visitor_new(bool human, char **result)
> >      v->visitor.end_list = end_list;
> >      v->visitor.complete = string_output_complete;
> >      v->visitor.free = string_output_free;
> > +    v->visitor.start_struct = start_struct;
> > +    v->visitor.end_struct = end_struct;
> >  
> >      return &v->visitor;
> >  }
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> 
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Avoid a segfault when visiting, e.g., the QOM rtc-time property,
>> > by implementing the struct callbacks and raising an Error.
>> >
>> > Signed-off-by: Andreas Färber <afaerber@suse.de>
>> >
>> > Updated for changed interface:
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > ---
>> >  qapi/string-output-visitor.c | 13 +++++++++++++
>> >  1 file changed, 13 insertions(+)
>> >
>> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
>> > index 94ac821..4e7e97f 100644
>> > --- a/qapi/string-output-visitor.c
>> > +++ b/qapi/string-output-visitor.c
>> > @@ -12,6 +12,7 @@
>> >  
>> >  #include "qemu/osdep.h"
>> >  #include "qemu-common.h"
>> > +#include "qapi/error.h"
>> >  #include "qapi/string-output-visitor.h"
>> >  #include "qapi/visitor-impl.h"
>> >  #include "qemu/host-utils.h"
>> > @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
>> >      string_output_set(sov, g_strdup_printf("%f", *obj));
>> >  }
>> >  
>> > +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
>> > +           Error **errp)
>> > +{
>> > +    error_setg(errp, "struct type not implemented");
>> > +}
>> > +
>> > +static void end_struct(Visitor *v, void **obj)
>> > +{
>> > +}
>> > +
>> 
>> This is just one of the several things this visitor doesn't implement.
>> See the comment in string-output-visitor.h.
>> 
>> String input visitor and options visitor have similar holes; see the
>> comments in string-input-visitor.h and opts-visitor.h.
>> 
>> Should we change all of them together to report errors instead of crash?
>> With common "error out because this isn't implemented" methods?
>
> In that case wouldn't it be best to change visit_start_struct/visit_end_struct
> to do the check (Like visit_check_struct does).

In my opinion.

    if (v->foo) {
        v->foo(...);
    } else {
        ... default action ...
    }

is an anti-pattern.  Wrap the default action in a default method, and
put that in the function pointer.

Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >> 
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >
> >> > Avoid a segfault when visiting, e.g., the QOM rtc-time property,
> >> > by implementing the struct callbacks and raising an Error.
> >> >
> >> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> >
> >> > Updated for changed interface:
> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> > ---
> >> >  qapi/string-output-visitor.c | 13 +++++++++++++
> >> >  1 file changed, 13 insertions(+)
> >> >
> >> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> >> > index 94ac821..4e7e97f 100644
> >> > --- a/qapi/string-output-visitor.c
> >> > +++ b/qapi/string-output-visitor.c
> >> > @@ -12,6 +12,7 @@
> >> >  
> >> >  #include "qemu/osdep.h"
> >> >  #include "qemu-common.h"
> >> > +#include "qapi/error.h"
> >> >  #include "qapi/string-output-visitor.h"
> >> >  #include "qapi/visitor-impl.h"
> >> >  #include "qemu/host-utils.h"
> >> > @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
> >> >      string_output_set(sov, g_strdup_printf("%f", *obj));
> >> >  }
> >> >  
> >> > +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
> >> > +           Error **errp)
> >> > +{
> >> > +    error_setg(errp, "struct type not implemented");
> >> > +}
> >> > +
> >> > +static void end_struct(Visitor *v, void **obj)
> >> > +{
> >> > +}
> >> > +
> >> 
> >> This is just one of the several things this visitor doesn't implement.
> >> See the comment in string-output-visitor.h.
> >> 
> >> String input visitor and options visitor have similar holes; see the
> >> comments in string-input-visitor.h and opts-visitor.h.
> >> 
> >> Should we change all of them together to report errors instead of crash?
> >> With common "error out because this isn't implemented" methods?
> >
> > In that case wouldn't it be best to change visit_start_struct/visit_end_struct
> > to do the check (Like visit_check_struct does).
> 
> In my opinion.
> 
>     if (v->foo) {
>         v->foo(...);
>     } else {
>         ... default action ...
>     }
> 
> is an anti-pattern.  Wrap the default action in a default method, and
> put that in the function pointer.

I've got some sympathy to that, but with the way our visitors are
built that's a pain.

Lets say you add a new eat_struct method, and a eat_struct_default implementation,
now you have to go around and fix all the visitor implementations to initialise
their eat_struct member to eat_struct_default.   Of course you'll forget some
and then we'll end up segging when you fall down the NULL pointer.

Now, if our visitors had nice shared constructor functions that wouldn't
be a problem, and you wouldn't need most of the visit_ wrapper functions;
but they don't, so the if (v->foo) { ... } else { error; }   is the
current cleanest we can do.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> 
>> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> >> 
>> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >> >
>> >> > Avoid a segfault when visiting, e.g., the QOM rtc-time property,
>> >> > by implementing the struct callbacks and raising an Error.
>> >> >
>> >> > Signed-off-by: Andreas Färber <afaerber@suse.de>
>> >> >
>> >> > Updated for changed interface:
>> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >> > ---
>> >> >  qapi/string-output-visitor.c | 13 +++++++++++++
>> >> >  1 file changed, 13 insertions(+)
>> >> >
>> >> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
>> >> > index 94ac821..4e7e97f 100644
>> >> > --- a/qapi/string-output-visitor.c
>> >> > +++ b/qapi/string-output-visitor.c
>> >> > @@ -12,6 +12,7 @@
>> >> >  
>> >> >  #include "qemu/osdep.h"
>> >> >  #include "qemu-common.h"
>> >> > +#include "qapi/error.h"
>> >> >  #include "qapi/string-output-visitor.h"
>> >> >  #include "qapi/visitor-impl.h"
>> >> >  #include "qemu/host-utils.h"
>> >> > @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
>> >> >      string_output_set(sov, g_strdup_printf("%f", *obj));
>> >> >  }
>> >> >  
>> >> > +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
>> >> > +           Error **errp)
>> >> > +{
>> >> > +    error_setg(errp, "struct type not implemented");
>> >> > +}
>> >> > +
>> >> > +static void end_struct(Visitor *v, void **obj)
>> >> > +{
>> >> > +}
>> >> > +
>> >> 
>> >> This is just one of the several things this visitor doesn't implement.
>> >> See the comment in string-output-visitor.h.
>> >> 
>> >> String input visitor and options visitor have similar holes; see the
>> >> comments in string-input-visitor.h and opts-visitor.h.
>> >> 
>> >> Should we change all of them together to report errors instead of crash?
>> >> With common "error out because this isn't implemented" methods?
>> >
>> > In that case wouldn't it be best to change visit_start_struct/visit_end_struct
>> > to do the check (Like visit_check_struct does).
>> 
>> In my opinion.
>> 
>>     if (v->foo) {
>>         v->foo(...);
>>     } else {
>>         ... default action ...
>>     }
>> 
>> is an anti-pattern.  Wrap the default action in a default method, and
>> put that in the function pointer.
>
> I've got some sympathy to that, but with the way our visitors are
> built that's a pain.
>
> Lets say you add a new eat_struct method, and a eat_struct_default implementation,
> now you have to go around and fix all the visitor implementations to initialise
> their eat_struct member to eat_struct_default.   Of course you'll forget some
> and then we'll end up segging when you fall down the NULL pointer.
>
> Now, if our visitors had nice shared constructor functions that wouldn't
> be a problem, and you wouldn't need most of the visit_ wrapper functions;
> but they don't, so the if (v->foo) { ... } else { error; }   is the
> current cleanest we can do.

Well, it's the cleanest we can do as long as we constrain ourselves not
to do much :)

We currently have seven visitors.  Every single one defines a
FOO_visitor_new() function that basically looks like this:

    Visitor *FOO_visitor_new(... whatever ...)
    {
        FOOVisitor v = g_malloc0(sizeof(*v));

        v->visitor.type = ...
        ... initialize more of v->visitor ...
        ... initialize other members of *v, if any ...

        return &v->visitor;
    }

I grant you that putting sensible defaults into v->visitor by
initializing them correctly in all the FOO_visitor_new() functions is a
bit of pain.  Not much pain; there are only seven.  Anyway, there are
several obvious ways to do this without pain:

(1) Have a visitor core function to set the defaults, call it first.

(2) Replace g_malloc0() by a visitor core function that additionally
    sets the defaults.  Basically fusing g_malloc0() into (1)'s
    function.

(3) Have a visitor core function that replaces null methods by defaults,
    and call it last.  This function can also check you filled out in
    the mandatory bits.  Have it return the visitor, so you can make it
    a tail call: return visitor_check(&v->visitor).

Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >> 
> >> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >> >> 
> >> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >> >
> >> >> > Avoid a segfault when visiting, e.g., the QOM rtc-time property,
> >> >> > by implementing the struct callbacks and raising an Error.
> >> >> >
> >> >> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> >> >
> >> >> > Updated for changed interface:
> >> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> >> > ---
> >> >> >  qapi/string-output-visitor.c | 13 +++++++++++++
> >> >> >  1 file changed, 13 insertions(+)
> >> >> >
> >> >> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> >> >> > index 94ac821..4e7e97f 100644
> >> >> > --- a/qapi/string-output-visitor.c
> >> >> > +++ b/qapi/string-output-visitor.c
> >> >> > @@ -12,6 +12,7 @@
> >> >> >  
> >> >> >  #include "qemu/osdep.h"
> >> >> >  #include "qemu-common.h"
> >> >> > +#include "qapi/error.h"
> >> >> >  #include "qapi/string-output-visitor.h"
> >> >> >  #include "qapi/visitor-impl.h"
> >> >> >  #include "qemu/host-utils.h"
> >> >> > @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
> >> >> >      string_output_set(sov, g_strdup_printf("%f", *obj));
> >> >> >  }
> >> >> >  
> >> >> > +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
> >> >> > +           Error **errp)
> >> >> > +{
> >> >> > +    error_setg(errp, "struct type not implemented");
> >> >> > +}
> >> >> > +
> >> >> > +static void end_struct(Visitor *v, void **obj)
> >> >> > +{
> >> >> > +}
> >> >> > +
> >> >> 
> >> >> This is just one of the several things this visitor doesn't implement.
> >> >> See the comment in string-output-visitor.h.
> >> >> 
> >> >> String input visitor and options visitor have similar holes; see the
> >> >> comments in string-input-visitor.h and opts-visitor.h.
> >> >> 
> >> >> Should we change all of them together to report errors instead of crash?
> >> >> With common "error out because this isn't implemented" methods?
> >> >
> >> > In that case wouldn't it be best to change visit_start_struct/visit_end_struct
> >> > to do the check (Like visit_check_struct does).
> >> 
> >> In my opinion.
> >> 
> >>     if (v->foo) {
> >>         v->foo(...);
> >>     } else {
> >>         ... default action ...
> >>     }
> >> 
> >> is an anti-pattern.  Wrap the default action in a default method, and
> >> put that in the function pointer.
> >
> > I've got some sympathy to that, but with the way our visitors are
> > built that's a pain.
> >
> > Lets say you add a new eat_struct method, and a eat_struct_default implementation,
> > now you have to go around and fix all the visitor implementations to initialise
> > their eat_struct member to eat_struct_default.   Of course you'll forget some
> > and then we'll end up segging when you fall down the NULL pointer.
> >
> > Now, if our visitors had nice shared constructor functions that wouldn't
> > be a problem, and you wouldn't need most of the visit_ wrapper functions;
> > but they don't, so the if (v->foo) { ... } else { error; }   is the
> > current cleanest we can do.
> 
> Well, it's the cleanest we can do as long as we constrain ourselves not
> to do much :)

Yes, although I hate to turn a patchset for a tiny feature into a
fix-all-the-broken-stuff set!

> We currently have seven visitors.  Every single one defines a
> FOO_visitor_new() function that basically looks like this:
> 
>     Visitor *FOO_visitor_new(... whatever ...)
>     {
>         FOOVisitor v = g_malloc0(sizeof(*v));
> 
>         v->visitor.type = ...
>         ... initialize more of v->visitor ...
>         ... initialize other members of *v, if any ...
> 
>         return &v->visitor;
>     }
> 
> I grant you that putting sensible defaults into v->visitor by
> initializing them correctly in all the FOO_visitor_new() functions is a
> bit of pain.  Not much pain; there are only seven.  Anyway, there are
> several obvious ways to do this without pain:
> 
> (1) Have a visitor core function to set the defaults, call it first.
> 
> (2) Replace g_malloc0() by a visitor core function that additionally
>     sets the defaults.  Basically fusing g_malloc0() into (1)'s
>     function.

That's my preferred way of doing it, chaining constructors.

Dave

> (3) Have a visitor core function that replaces null methods by defaults,
>     and call it last.  This function can also check you filled out in
>     the mandatory bits.  Have it return the visitor, so you can make it
>     a tail call: return visitor_check(&v->visitor).
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> 
>> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> >> 
>> >> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> >> >> 
>> >> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >> >> >
>> >> >> > Avoid a segfault when visiting, e.g., the QOM rtc-time property,
>> >> >> > by implementing the struct callbacks and raising an Error.
>> >> >> >
>> >> >> > Signed-off-by: Andreas Färber <afaerber@suse.de>
>> >> >> >
>> >> >> > Updated for changed interface:
>> >> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >> >> > ---
>> >> >> >  qapi/string-output-visitor.c | 13 +++++++++++++
>> >> >> >  1 file changed, 13 insertions(+)
>> >> >> >
>> >> >> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
>> >> >> > index 94ac821..4e7e97f 100644
>> >> >> > --- a/qapi/string-output-visitor.c
>> >> >> > +++ b/qapi/string-output-visitor.c
>> >> >> > @@ -12,6 +12,7 @@
>> >> >> >  
>> >> >> >  #include "qemu/osdep.h"
>> >> >> >  #include "qemu-common.h"
>> >> >> > +#include "qapi/error.h"
>> >> >> >  #include "qapi/string-output-visitor.h"
>> >> >> >  #include "qapi/visitor-impl.h"
>> >> >> >  #include "qemu/host-utils.h"
>> >> >> > @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
>> >> >> >      string_output_set(sov, g_strdup_printf("%f", *obj));
>> >> >> >  }
>> >> >> >  
>> >> >> > +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
>> >> >> > +           Error **errp)
>> >> >> > +{
>> >> >> > +    error_setg(errp, "struct type not implemented");
>> >> >> > +}
>> >> >> > +
>> >> >> > +static void end_struct(Visitor *v, void **obj)
>> >> >> > +{
>> >> >> > +}
>> >> >> > +
>> >> >> 
>> >> >> This is just one of the several things this visitor doesn't implement.
>> >> >> See the comment in string-output-visitor.h.
>> >> >> 
>> >> >> String input visitor and options visitor have similar holes; see the
>> >> >> comments in string-input-visitor.h and opts-visitor.h.
>> >> >> 
>> >> >> Should we change all of them together to report errors instead of crash?
>> >> >> With common "error out because this isn't implemented" methods?
>> >> >
>> >> > In that case wouldn't it be best to change visit_start_struct/visit_end_struct
>> >> > to do the check (Like visit_check_struct does).
>> >> 
>> >> In my opinion.
>> >> 
>> >>     if (v->foo) {
>> >>         v->foo(...);
>> >>     } else {
>> >>         ... default action ...
>> >>     }
>> >> 
>> >> is an anti-pattern.  Wrap the default action in a default method, and
>> >> put that in the function pointer.
>> >
>> > I've got some sympathy to that, but with the way our visitors are
>> > built that's a pain.
>> >
>> > Lets say you add a new eat_struct method, and a eat_struct_default implementation,
>> > now you have to go around and fix all the visitor implementations to initialise
>> > their eat_struct member to eat_struct_default.   Of course you'll forget some
>> > and then we'll end up segging when you fall down the NULL pointer.
>> >
>> > Now, if our visitors had nice shared constructor functions that wouldn't
>> > be a problem, and you wouldn't need most of the visit_ wrapper functions;
>> > but they don't, so the if (v->foo) { ... } else { error; }   is the
>> > current cleanest we can do.
>> 
>> Well, it's the cleanest we can do as long as we constrain ourselves not
>> to do much :)
>
> Yes, although I hate to turn a patchset for a tiny feature into a
> fix-all-the-broken-stuff set!

I know the feeling...

I'd love to accommodate you, but I'm afraid the work is too incomplete
in its current state.  The string output visitor doesn't implement a
number of things besides structs.  To convince me that your qom-get
won't crash because of that, you'd have to show that these other things
cannot happen with qom-get.  Implementing the missing parts instead
would probably be easier.  And then one of the general solutions
discussed below would hardly be more work, for more value.

>> We currently have seven visitors.  Every single one defines a
>> FOO_visitor_new() function that basically looks like this:
>> 
>>     Visitor *FOO_visitor_new(... whatever ...)
>>     {
>>         FOOVisitor v = g_malloc0(sizeof(*v));
>> 
>>         v->visitor.type = ...
>>         ... initialize more of v->visitor ...
>>         ... initialize other members of *v, if any ...
>> 
>>         return &v->visitor;
>>     }
>> 
>> I grant you that putting sensible defaults into v->visitor by
>> initializing them correctly in all the FOO_visitor_new() functions is a
>> bit of pain.  Not much pain; there are only seven.  Anyway, there are
>> several obvious ways to do this without pain:
>> 
>> (1) Have a visitor core function to set the defaults, call it first.
>> 
>> (2) Replace g_malloc0() by a visitor core function that additionally
>>     sets the defaults.  Basically fusing g_malloc0() into (1)'s
>>     function.
>
> That's my preferred way of doing it, chaining constructors.

It's a fine way of doing it when you only ever create the things in one
way.  Here, with g_malloc0().

Would you like to wait for the Dan's visitor work?  Perhaps the problem
goes away there...

> Dave
>
>> (3) Have a visitor core function that replaces null methods by defaults,
>>     and call it last.  This function can also check you filled out in
>>     the mandatory bits.  Have it return the visitor, so you can make it
>>     a tail call: return visitor_check(&v->visitor).
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] qapi: Stub out StringOutputVisitor struct support

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >> 
> >> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >> >> 
> >> >> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> >> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >> >> >> 
> >> >> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >> >> >
> >> >> >> > Avoid a segfault when visiting, e.g., the QOM rtc-time property,
> >> >> >> > by implementing the struct callbacks and raising an Error.
> >> >> >> >
> >> >> >> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> >> >> >
> >> >> >> > Updated for changed interface:
> >> >> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> >> >> > ---
> >> >> >> >  qapi/string-output-visitor.c | 13 +++++++++++++
> >> >> >> >  1 file changed, 13 insertions(+)
> >> >> >> >
> >> >> >> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> >> >> >> > index 94ac821..4e7e97f 100644
> >> >> >> > --- a/qapi/string-output-visitor.c
> >> >> >> > +++ b/qapi/string-output-visitor.c
> >> >> >> > @@ -12,6 +12,7 @@
> >> >> >> >  
> >> >> >> >  #include "qemu/osdep.h"
> >> >> >> >  #include "qemu-common.h"
> >> >> >> > +#include "qapi/error.h"
> >> >> >> >  #include "qapi/string-output-visitor.h"
> >> >> >> >  #include "qapi/visitor-impl.h"
> >> >> >> >  #include "qemu/host-utils.h"
> >> >> >> > @@ -266,6 +267,16 @@ static void print_type_number(Visitor *v, const char *name, double *obj,
> >> >> >> >      string_output_set(sov, g_strdup_printf("%f", *obj));
> >> >> >> >  }
> >> >> >> >  
> >> >> >> > +static void start_struct(Visitor *v, const char *name, void **obj, size_t size,
> >> >> >> > +           Error **errp)
> >> >> >> > +{
> >> >> >> > +    error_setg(errp, "struct type not implemented");
> >> >> >> > +}
> >> >> >> > +
> >> >> >> > +static void end_struct(Visitor *v, void **obj)
> >> >> >> > +{
> >> >> >> > +}
> >> >> >> > +
> >> >> >> 
> >> >> >> This is just one of the several things this visitor doesn't implement.
> >> >> >> See the comment in string-output-visitor.h.
> >> >> >> 
> >> >> >> String input visitor and options visitor have similar holes; see the
> >> >> >> comments in string-input-visitor.h and opts-visitor.h.
> >> >> >> 
> >> >> >> Should we change all of them together to report errors instead of crash?
> >> >> >> With common "error out because this isn't implemented" methods?
> >> >> >
> >> >> > In that case wouldn't it be best to change visit_start_struct/visit_end_struct
> >> >> > to do the check (Like visit_check_struct does).
> >> >> 
> >> >> In my opinion.
> >> >> 
> >> >>     if (v->foo) {
> >> >>         v->foo(...);
> >> >>     } else {
> >> >>         ... default action ...
> >> >>     }
> >> >> 
> >> >> is an anti-pattern.  Wrap the default action in a default method, and
> >> >> put that in the function pointer.
> >> >
> >> > I've got some sympathy to that, but with the way our visitors are
> >> > built that's a pain.
> >> >
> >> > Lets say you add a new eat_struct method, and a eat_struct_default implementation,
> >> > now you have to go around and fix all the visitor implementations to initialise
> >> > their eat_struct member to eat_struct_default.   Of course you'll forget some
> >> > and then we'll end up segging when you fall down the NULL pointer.
> >> >
> >> > Now, if our visitors had nice shared constructor functions that wouldn't
> >> > be a problem, and you wouldn't need most of the visit_ wrapper functions;
> >> > but they don't, so the if (v->foo) { ... } else { error; }   is the
> >> > current cleanest we can do.
> >> 
> >> Well, it's the cleanest we can do as long as we constrain ourselves not
> >> to do much :)
> >
> > Yes, although I hate to turn a patchset for a tiny feature into a
> > fix-all-the-broken-stuff set!
> 
> I know the feeling...
> 
> I'd love to accommodate you, but I'm afraid the work is too incomplete
> in its current state.  The string output visitor doesn't implement a
> number of things besides structs.  To convince me that your qom-get
> won't crash because of that, you'd have to show that these other things
> cannot happen with qom-get.  Implementing the missing parts instead
> would probably be easier.  And then one of the general solutions
> discussed below would hardly be more work, for more value.
> 
> >> We currently have seven visitors.  Every single one defines a
> >> FOO_visitor_new() function that basically looks like this:
> >> 
> >>     Visitor *FOO_visitor_new(... whatever ...)
> >>     {
> >>         FOOVisitor v = g_malloc0(sizeof(*v));
> >> 
> >>         v->visitor.type = ...
> >>         ... initialize more of v->visitor ...
> >>         ... initialize other members of *v, if any ...
> >> 
> >>         return &v->visitor;
> >>     }
> >> 
> >> I grant you that putting sensible defaults into v->visitor by
> >> initializing them correctly in all the FOO_visitor_new() functions is a
> >> bit of pain.  Not much pain; there are only seven.  Anyway, there are
> >> several obvious ways to do this without pain:
> >> 
> >> (1) Have a visitor core function to set the defaults, call it first.
> >> 
> >> (2) Replace g_malloc0() by a visitor core function that additionally
> >>     sets the defaults.  Basically fusing g_malloc0() into (1)'s
> >>     function.
> >
> > That's my preferred way of doing it, chaining constructors.
> 
> It's a fine way of doing it when you only ever create the things in one
> way.  Here, with g_malloc0().
> 
> Would you like to wait for the Dan's visitor work?  Perhaps the problem
> goes away there...

Will it ?

Dave

> > Dave
> >
> >> (3) Have a visitor core function that replaces null methods by defaults,
> >>     and call it last.  This function can also check you filled out in
> >>     the mandatory bits.  Have it return the visitor, so you can make it
> >>     a tail call: return visitor_check(&v->visitor).
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK