Re: [PATCH RFC 08/10] vmstate: Implement load of ptr marker in vmstate core

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> The loader side of ptr marker is pretty straightforward, instead of playing
> the inner_field trick, just do the load manually assuming the marker layout
> is a stable ABI (which it is true already).
>
> This will remove some logic while loading VMSD, and hopefully it makes it
> slightly easier to read.  Unfortunately, we still need to keep the sender
> side because of the JSON blob we're maintaining..
>

/ramble
Is the JSON blob ABI? Can we kill it? AFAIR, it only serves to add
complexity and to break analyze-script from time to time. We'd be better
off parsing the actual stream from a file. Could maybe even use the same
loadvm code, but mock the .get functions to print instead of actually
loading.

Having separate code that parses a "thing" that's not the stream, but
that should reflect the stream, but it's not the stream, but pretty
close it's a bit weird to me. I recently had to simply open the raw
stream on emacs and navigate through it because the file: stream was
somehow different from the stream on the qcow2, for the same command
line.

(that's another point, parsing from the qcow2 would be cool, which the
JSON blob doesn't provide today I think)
ramble/

> This paves way for future processing of non-NULL markers as well.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/vmstate-types.c | 12 ++++--------
>  migration/vmstate.c       | 40 ++++++++++++++++++++++++---------------
>  2 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index b31689fc3c..ae465c5c2c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
>                              const VMStateField *field, Error **errp)
>  
>  {
> -    int byte = qemu_get_byte(f);
> -
> -    if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) {
> -        /* TODO: process PTR_VALID case */
> -        return true;
> -    }
> -
> -    error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> +    /*
> +     * Load is done in vmstate core, see vmstate_ptr_marker_load().
> +     */
> +    g_assert_not_reached();
>      return false;
>  }
>  
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index a3a5f25946..d65fc84dfa 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
>      }
>  }
>  
> +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> +                                    Error **errp)
> +{
> +    int byte = qemu_get_byte(f);
> +
> +    if (byte == VMS_MARKER_PTR_NULL) {
> +        /* When it's a null ptr marker, do not continue the load */
> +        *load_field = false;
> +        return true;
> +    }
> +
> +    error_setg(errp, "Unexpected ptr marker: %d", byte);
> +    return false;
> +}
> +
>  static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque,
>                               Error **errp)
>  {
> @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>              }
>  
>              for (i = 0; i < n_elems; i++) {
> -                bool ok;
> +                /* If we will process the load of field? */
> +                bool load_field = true;

maybe valid_ptr would be more clear?

> +                bool ok = true;
>                  void *curr_elem = first_elem + size * i;
> -                const VMStateField *inner_field;
>  
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>                      curr_elem = *(void **)curr_elem;
>                  }
>  
>                  if (!curr_elem && size) {
> -                    /*
> -                     * If null pointer found (which should only happen in
> -                     * an array of pointers), use null placeholder and do
> -                     * not follow.
> -                     */
> -                    inner_field = vmsd_create_ptr_marker_field(field);
> -                } else {
> -                    inner_field = field;
> +                    /* Read the marker instead of VMSD itself */
> +                    if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> +                        trace_vmstate_load_field_error(field->name, -EINVAL);
> +                        return false;
> +                    }
>                  }
>  
> -                ok = vmstate_load_field(f, curr_elem, size, inner_field, errp);
> -
> -                /* If we used a fake temp field.. free it now */
> -                if (inner_field != field) {
> -                    g_clear_pointer((gpointer *)&inner_field, g_free);
> +                if (load_field) {
> +                    ok = vmstate_load_field(f, curr_elem, size, field, errp);
>                  }
>  
>                  if (ok) {