[v14,03/19] qapi: Guarantee NULL obj on input visitor callback error

Our existing input visitors were not very consistent on errors
in a function taking 'TYPE **obj'. While all of them set '*obj'
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.

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 <eblake@redhat.com>

---
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(-)

Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>
> ---
> 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.