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

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Mar 19, 2026 at 05:46:36PM -0300, Fabiano Rosas wrote:
>> 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.
>> >
>> > 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         | 29 ++++++++++++++---------------
>> >  3 files changed, 37 insertions(+), 35 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 5a6b352764..a3a5f25946 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);
>> >  
>> > @@ -76,7 +76,7 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
>> >  
>> >      /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
>> 
>> This comment needs updating.
>
> Fixed.
>
>> 
>> >      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;
>> >                  }
>> > @@ -567,7 +567,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>> >              int i, n_elems = vmstate_n_elems(opaque, field);
>> >              int size = vmstate_size(opaque, field);
>> >              JSONWriter *vmdesc_loop = vmdesc;
>> > -            bool is_prev_null = false;
>> > +            bool use_marker_field_prev = false;
>> 
>> The logic below won't handle valid pointer as well, will it? We could
>> leave the is_null/is_prev_null terminology because it's way more
>> descriptive. Right? We're adding the marker here because we're
>> compressing and know the field is null.
>
> I recovered the old bool for better readability.
>
> One other note is I'll squash below analyze-migration.py change into this
> patch when repost, let me know if there's early comments:
>
> ===8<===
>
> From 82ac38a85ecd990a1aaa7b45b172e71f0d5b84da Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Thu, 26 Mar 2026 14:57:56 -0400
> Subject: [PATCH] scripts/analyze-migration.py: Support new ptr-marker field
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  scripts/analyze-migration.py | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> 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,
>  }
>  

LGTM