Re: [Qemu-devel] [PATCH 12/24] qobject: Propagate parse errors through qobject_from_json()

[Eric Blake <eblake@redhat.com>]

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

On 02/27/2017 05:20 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                            |  2 +-
>  include/qapi/qmp/qjson.h           |  5 +--
>  monitor.c                          |  2 +-
>  qobject/qjson.c                    |  4 +--
>  tests/check-qjson.c                | 62 +++++++++++++++++++-------------------
>  tests/test-visitor-serialization.c |  2 +-
>  6 files changed, 39 insertions(+), 38 deletions(-)
> 

As with 8/24, this would be a good place for the commit message to
mention that this patch adds the parameter and mechanically adjusts
callers with minimal semantic changes, but that later patches will take
advantage of the parameter.

> +++ b/include/qapi/qmp/qjson.h
> @@ -17,8 +17,9 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/qmp/qstring.h"
>  
> -QObject *qobject_from_json(const char *string);
> -QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
> +QObject *qobject_from_json(const char *string, Error **errp);

The real change here, vs.

> +QObject *qobject_from_jsonf(const char *string, ...)
> +    GCC_FMT_ATTR(1, 2);

formatting.  Should the formatting be hoisted earlier in the series,
when you first touch qobject_from_jsonv?

>  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>      GCC_FMT_ATTR(1, 0);

This makes qobject_from_jsonf() and qobject_from_jsonv() asymmetric
(only one of the two can report errors); and looks a bit weird to have a
va_list not as the last argument (as it is, a 'va_list *' argument is
already weird).  If symmetry is important, we can put errp prior to the
.../ap argument (then both forms have an errp pointer).  But I don't
think it matters in the context of this series.  If you DO change it,
though, then 8/24 would be the place to tweak it.

At one point, I posted a series that removed all uses of
qobject_from_json[fv] (leaving just qobject_from_json); at the time,
there was not a strong opinion on whether the series was worthwhile, but
if we want it, I'm fine rebasing on top of this.  (One argument in favor
of my series would be getting rid of the weird 'va_list *' argument).

> +++ b/qobject/qjson.c
> @@ -50,9 +50,9 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>      return state.result;
>  }
>  
> -QObject *qobject_from_json(const char *string)
> +QObject *qobject_from_json(const char *string, Error **errp)
>  {
> -    return qobject_from_jsonv(string, NULL, NULL);
> +    return qobject_from_jsonv(string, NULL, errp);

Of course, if you rebase the series to rearrange where errp appears in
relation to va_list, then be sure this changes to match (the compiler
may or may not flag it, if va_list looks too much like void*).

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]