Re: [PATCH RFC v2 07/11] vmstate: Allow vmstate_info_nullptr to emit non-NULL markers

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> We used to have one vmstate called "nullptr" which is only used to generate
> one-byte hint to say one pointer is NULL.
>
> Let's extend its use so that it will generate another byte to say the
> pointer is non-NULL.
>
> With that, the name of the info struct (or functions) do not apply anymore.
> Update correspondingly.
>
> Update analyze-migration.py to work with the new layout.
>
> No functional change intended yet.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/vmstate.h  |  9 +++++++--
>  migration/vmstate-types.c    | 34 ++++++++++++++++------------------
>  migration/vmstate.c          | 25 +++++++++++++------------
>  scripts/analyze-migration.py | 22 ++++++++++++----------
>  4 files changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 092e8f7e9a..2e51b5ea04 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
>  extern const VMStateInfo vmstate_info_uint64;
>  extern const VMStateInfo vmstate_info_fd;
>  
> -/** Put this in the stream when migrating a null pointer.*/
> +/*
> + * Put this in the stream when migrating a pointer to reflect either a NULL
> + * or valid pointer.
> + */
>  #define VMS_MARKER_PTR_NULL          (0x30U)   /* '0' */
> -extern const VMStateInfo vmstate_info_nullptr;
> +#define VMS_MARKER_PTR_VALID         (0x31U)   /* '1' */
> +
> +extern const VMStateInfo vmstate_info_ptr_marker;
>  
>  extern const VMStateInfo vmstate_info_cpudouble;
>  
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 7622cf8f01..b31689fc3c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
>      .save = save_fd,
>  };
>  
> -static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> -                         const VMStateField *field, Error **errp)
> +static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> +                            const VMStateField *field, Error **errp)
>  
>  {
> -    if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> +    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, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
> +    error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
>      return false;
>  }
>  
> -static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
> -                         const VMStateField *field, JSONWriter *vmdesc,
> -                         Error **errp)
> +static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
> +                            const VMStateField *field, JSONWriter *vmdesc,
> +                            Error **errp)
>  
>  {
> -    if (pv == NULL) {
> -        qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> -        return true;
> -    }
> -
> -    error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
> -    return false;
> +    qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
> +    return true;
>  }
>  
> -const VMStateInfo vmstate_info_nullptr = {
> -    .name = "nullptr",
> -    .load = load_nullptr,
> -    .save = save_nullptr,
> +const VMStateInfo vmstate_info_ptr_marker = {
> +    .name = "ptr-marker",
> +    .load = load_ptr_marker,
> +    .save = save_ptr_marker,
>  };
>  
>  /* 64 bit unsigned int. See that the received value is the same than the one
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index b274204e66..b333aa1744 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
>  }
>  
>  /*
> - * Create a fake nullptr field when there's a NULL pointer detected in the
> + * Create a ptr marker field when there's a NULL pointer detected in the
>   * array of a VMS_ARRAY_OF_POINTER VMSD field.  It's needed because we
>   * can't dereference the NULL pointer.
>   */
>  static const VMStateField *
> -vmsd_create_fake_nullptr_field(const VMStateField *field)
> +vmsd_create_ptr_marker_field(const VMStateField *field)
>  {
>      VMStateField *fake = g_new0(VMStateField, 1);
>  
> @@ -71,12 +71,12 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
>      fake->name = field->name;
>      fake->version_id = field->version_id;
>  
> -    /* Do not need "field_exists" check as it always exists (which is null) */
> +    /* Do not need "field_exists" check as it always exists */
>      fake->field_exists = NULL;
>  
> -    /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
> +    /* See vmstate_info_ptr_marker - use 1 byte to represent ptr status */
>      fake->size = 1;
> -    fake->info = &vmstate_info_nullptr;
> +    fake->info = &vmstate_info_ptr_marker;
>      fake->flags = VMS_SINGLE;
>  
>      /* All the rest fields shouldn't matter.. */
> @@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>                       * an array of pointers), use null placeholder and do
>                       * not follow.
>                       */
> -                    inner_field = vmsd_create_fake_nullptr_field(field);
> +                    inner_field = vmsd_create_ptr_marker_field(field);
>                  } else {
>                      inner_field = field;
>                  }
> @@ -583,26 +583,27 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>              for (i = 0; i < n_elems; i++) {
>                  void *curr_elem = first_elem + size * i;
>                  const VMStateField *inner_field;
> -                bool is_null;
>                  /* maximum number of elements to compress in the JSON blob */
>                  int max_elems = vmsd_can_compress(field) ? (n_elems - i) : 1;
> +                bool use_marker_field, is_null;
>  
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>                      assert(curr_elem);
>                      curr_elem = *(void **)curr_elem;
>                  }
>  
> -                if (!curr_elem && size) {
> +                is_null = !curr_elem && size;
> +                use_marker_field = is_null;
> +
> +                if (use_marker_field) {
>                      /*
>                       * If null pointer found (which should only happen in
>                       * an array of pointers), use null placeholder and do
>                       * not follow.
>                       */
> -                    inner_field = vmsd_create_fake_nullptr_field(field);
> -                    is_null = true;
> +                    inner_field = vmsd_create_ptr_marker_field(field);
>                  } else {
>                      inner_field = field;
> -                    is_null = false;
>                  }
>  
>                  /*
> @@ -638,7 +639,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>                                                      i, max_elems, errp);
>  
>                  /* If we used a fake temp field.. free it now */
> -                if (is_null) {
> +                if (use_marker_field) {
>                      g_clear_pointer((gpointer *)&inner_field, g_free);
>                  }
>  
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index e81deab8f9..1771ff781b 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -469,26 +469,26 @@ def __init__(self, desc, file):
>          super(VMSDFieldIntLE, self).__init__(desc, file)
>          self.dtype = '<i%d' % self.size
>  
> -class VMSDFieldNull(VMSDFieldGeneric):
> +class VMSDFieldPtrMarker(VMSDFieldGeneric):
>      NULL_PTR_MARKER = b'0'
> +    VALID_PTR_MARKER = b'1'
>  
>      def __init__(self, desc, file):
> -        super(VMSDFieldNull, self).__init__(desc, file)
> +        super(VMSDFieldPtrMarker, self).__init__(desc, file)
>  
>      def __repr__(self):
> -        # A NULL pointer is encoded in the stream as a '0' to
> -        # disambiguate from a mere 0x0 value and avoid consumers
> -        # trying to follow the NULL pointer. Displaying '0', 0x30 or
> -        # 0x0 when analyzing the JSON debug stream could become
> +        # A NULL / non-NULL pointer may be encoded in the stream as a
> +        # '0'/'1' to represent the status of the pointer.  Displaying '0',
> +        # 0x30 or 0x0 when analyzing the JSON debug stream could become
>          # confusing, so use an explicit term instead.
> -        return "nullptr"
> +        return "null-ptr" if self.data == self.NULL_PTR_MARKER else "valid-ptr"
>  
>      def __str__(self):
>          return self.__repr__()
>  
>      def read(self):
> -        super(VMSDFieldNull, self).read()
> -        assert(self.data == self.NULL_PTR_MARKER)
> +        super(VMSDFieldPtrMarker, self).read()
> +        assert(self.data in [self.NULL_PTR_MARKER, self.VALID_PTR_MARKER])
>          return self.data
>  
>  class VMSDFieldBool(VMSDFieldGeneric):
> @@ -642,7 +642,9 @@ def getDict(self):
>      "bitmap" : VMSDFieldGeneric,
>      "struct" : VMSDFieldStruct,
>      "capability": VMSDFieldCap,
> -    "nullptr": VMSDFieldNull,
> +    # Keep the old nullptr for old binaries
> +    "nullptr": VMSDFieldPtrMarker,
> +    "ptr-marker": VMSDFieldPtrMarker,
>      "unknown" : VMSDFieldGeneric,
>  }

Reviewed-by: Fabiano Rosas <farosas@suse.de>