Re: [Qemu-devel] [PATCH v14 03/19] qapi: Guarantee NULL obj on input visitor callback error

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Our existing input visitors were not very consistent on errors
> in a function taking 'TYPE **obj'. While all of them set '*obj'

Suggest to list the methods.  I guess it's start_struct(),
start_alternate(), type_str(), type_any().

> to allocated storage on success, it was not obvious whether
> '*obj' was guaranteed safe on failure, or whether it was left
> uninitialized.  But a future patch wants to guarantee that
> visit_type_FOO() does not leak a partially-constructed obj back
> to the caller; it is easier to implement this if we can reliably
> state that '*obj' is assigned on exit, even on failures.

There are two sane behaviors on failure: (1) don't touch *obj, (2) set
it to null.  I generally like "do nothing on failure", i.e. (1), but I'm
fine with (2) when it's more convenient.  We'll see.

> The opts-visitor start_struct() doesn't set an error, but it
> also was doing a weird check for 0 size; all callers pass in
> non-zero size if obj is non-NULL.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v14: no change
> v13: no change
> v12: new patch
> ---
>  qapi/opts-visitor.c         | 3 ++-
>  qapi/qmp-input-visitor.c    | 4 ++++
>  qapi/string-input-visitor.c | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index f98cf2e..cdb6e42 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -133,7 +133,7 @@ opts_start_struct(Visitor *v, const char *name, void 
> **obj,
>      const QemuOpt *opt;
>
>      if (obj) {
> -        *obj = g_malloc0(size > 0 ? size : 1);
> +        *obj = g_malloc0(size);
>      }
>      if (ov->depth++ > 0) {
>          return;
> @@ -314,6 +314,7 @@ opts_type_str(Visitor *v, const char *name, char **obj, 
> Error **errp)
>
>      opt = lookup_scalar(ov, name, errp);
>      if (!opt) {
> +        *obj = NULL;
>          return;
>      }
>      *obj = g_strdup(opt->str ? opt->str : "");
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 02d4233..77cce8b 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -120,6 +120,9 @@ static void qmp_input_start_struct(Visitor *v, const char 
> *name, void **obj,
>      QObject *qobj = qmp_input_get_object(qiv, name, true);
>      Error *err = NULL;
>
> +    if (obj) {
> +        *obj = NULL;
> +    }
>      if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "QDict");
> @@ -267,6 +270,7 @@ static void qmp_input_type_str(Visitor *v, const char 
> *name, char **obj,
>      QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, 
> true));
>
>      if (!qstr) {
> +        *obj = NULL;
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "string");
>          return;
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index d604575..797973a 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -293,6 +293,7 @@ static void parse_type_str(Visitor *v, const char *name, 
> char **obj,
>      if (siv->string) {
>          *obj = g_strdup(siv->string);
>      } else {
> +        *obj = NULL;
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "string");
>      }

If you want to make setting *obj on failure part of the method contract,
you get to write it into the contract.  Looks like you do that later in
this series, when you retrofit the missing contracts.  Okay.