Re: [Qemu-devel] [PATCH v3 10/18] vmstate: Use new JSON output visitor

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Rather than using a QJSON object and converting the QString result
> to a char *, we can use the new JSON output visitor and get directly
> to a char *.
>
> The conversions are a bit tricky in place (in places, we have to
> copy an integer to an int64_t temporary to get the right pointer for
> visit_type_int(); and for several strings, we have to copy to a
> temporary variable so we can take an address (&char[] is not the
> same as &char*) and cast away const), but overall still fairly
> mechanical.
>
> Suggested-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v3: retitle, rebase to master
> v2: new patch
> ---
>  include/migration/vmstate.h |  4 +--
>  migration/savevm.c          | 68 
> ++++++++++++++++++++++++++++-----------------
>  migration/vmstate.c         | 64 ++++++++++++++++++++++++++----------------
>  tests/Makefile              |  2 +-
>  4 files changed, 86 insertions(+), 52 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 84ee355..2cdfce9 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -29,7 +29,7 @@
>  #ifndef CONFIG_USER_ONLY
>  #include <migration/qemu-file.h>
>  #endif
> -#include <qjson.h>
> +#include "qapi/json-output-visitor.h"
>
>  typedef void SaveStateHandler(QEMUFile *f, void *opaque);
>  typedef int LoadStateHandler(QEMUFile *f, void *opaque, int version_id);
> @@ -925,7 +925,7 @@ void loadvm_free_handlers(MigrationIncomingState *mis);
>  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                         void *opaque, int version_id);
>  void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> -                        void *opaque, QJSON *vmdesc);
> +                        void *opaque, Visitor *vmdesc);
>
>  bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 16ba443..c858fe9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2,7 +2,7 @@
>   * QEMU System Emulator
>   *
>   * Copyright (c) 2003-2008 Fabrice Bellard
> - * Copyright (c) 2009-2015 Red Hat Inc
> + * Copyright (c) 2009-2016 Red Hat Inc
>   *
>   * Authors:
>   *  Juan Quintela <address@hidden>
> @@ -645,27 +645,32 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry 
> *se, int version_id)
>      return vmstate_load_state(f, se->vmsd, se->opaque, version_id);
>  }
>
> -static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se, QJSON 
> *vmdesc)
> +static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> +                                   Visitor *vmdesc)
>  {
>      int64_t old_offset, size;
> +    const char *tmp;
>
>      old_offset = qemu_ftell_fast(f);
>      se->ops->save_state(f, se->opaque);
>      size = qemu_ftell_fast(f) - old_offset;
>
>      if (vmdesc) {

Conditionals could be avoided: use a null visitor.  Not sure it's worth
it, though.

> -        json_prop_int(vmdesc, "size", size);
> -        json_start_array(vmdesc, "fields");
> -        json_start_object(vmdesc, NULL);
> -        json_prop_str(vmdesc, "name", "data");
> -        json_prop_int(vmdesc, "size", size);
> -        json_prop_str(vmdesc, "type", "buffer");
> -        json_end_object(vmdesc);
> -        json_end_array(vmdesc);
> +        visit_type_int(vmdesc, "size", &size, &error_abort);
> +        visit_start_list(vmdesc, "fields", NULL, 0, &error_abort);
> +        visit_start_struct(vmdesc, NULL, NULL, 0, &error_abort);
> +        tmp = "data";
> +        visit_type_str(vmdesc, "name", (char **)&tmp, &error_abort);

The Visitor interface is the same for input and for output.  Convenient
when the code is direction-agnostic.  Inconvenient when it's output: you
have to pass the value by reference even though it's only read.  In
particular, literals need a temporary, and types have to be adjusted via
cast or temporary more frequently than for by-value.

If that bothers us, we can add by-value wrappers to the interface.

Are there other output-only visitor uses?

> +        visit_type_int(vmdesc, "size", &size, &error_abort);
> +        tmp = "buffer";
> +        visit_type_str(vmdesc, "type", (char **)&tmp, &error_abort);
> +        visit_check_struct(vmdesc, &error_abort);
> +        visit_end_struct(vmdesc);
> +        visit_end_list(vmdesc);
>      }
>  }
>
> -static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
> +static void vmstate_save(QEMUFile *f, SaveStateEntry *se, Visitor *vmdesc)
>  {
>      trace_vmstate_save(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
>      if (!se->vmsd) {
> @@ -891,7 +896,7 @@ void qemu_savevm_state_header(QEMUFile *f)
>
>      if (!savevm_state.skip_configuration || enforce_config_section()) {
>          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
> -        vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
> +        vmstate_save_state(f, &vmstate_configuration, &savevm_state, NULL);

Cleans up use of 0 as pointer literal while there, good.

Note to self: use Coccinelle to find this style bug's buddies.

>      }
>
>  }
[...]

Well, it doesn't exactly make this code prettier, but having a stupid
wrapper just to hide the ugliness isn't so hot, either.