Eric Blake <ebl...@redhat.com> 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 <pbonz...@redhat.com>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> 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 <quint...@redhat.com>
> @@ -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.
