Re: [PATCH 3/7] migration: Document the effect of vmstate_info_nullptr

[Peter Xu]

On Tue, Jan 07, 2025 at 04:50:21PM -0300, Fabiano Rosas wrote:
> The migration stream lacks magic numbers at some key points. It's easy
> to mis-parse data. Unfortunately, the VMS_NULLPTR_MARKER continues
> with the trend. A '0' byte is ambiguous and could be interpreted as a
> valid 0x30.
> 
> It is maybe not worth trying to change this while keeping backward
> compatibility, so add some words of documentation to clarify.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/vmstate-types.c    | 6 ++++++
>  scripts/analyze-migration.py | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index e83bfccb9e..08ed059f87 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -339,6 +339,12 @@ static int put_nullptr(QEMUFile *f, void *pv, size_t 
> size,
>  
>  const VMStateInfo vmstate_info_nullptr = {
>      .name = "uint64",

Ouch.. So I overlooked this line and this explains why it didn't go via
VMSDFieldGeneric already.

Instead of below comment, do we still have chance to change this to
something like "uint8"?  Then I suppose the script will be able to identify
this properly.

> +
> +     /*
> +      * Ideally these would actually read/write the size of a pointer,
> +      * but we're stuck with just a byte now for backward
> +      * compatibility.
> +      */
>      .get  = get_nullptr,
>      .put  = put_nullptr,
>  };
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index f2457b1dde..4292fde424 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -388,12 +388,21 @@ def read(self):
>          return self.data
>  
>  class VMSDFieldUInt(VMSDFieldInt):
> +    NULL_PTR_MARKER = 0x30
> +
>      def __init__(self, desc, file):
>          super(VMSDFieldUInt, self).__init__(desc, file)
>  
>      def read(self):
>          super(VMSDFieldUInt, self).read()
>          self.data = self.udata
> +
> +        if self.data == self.NULL_PTR_MARKER:
> +            # The migration stream encodes NULL pointers as '0' so any
> +            # 0x30 in the stream could be a NULL. There's not much we
> +            # can do without breaking backward compatibility.
> +            pass

So this change doesn't do anything, right?

It'll be weird here having it "uint64" but the super().read() will actually
only read 1 byte..  I assume the oneliner change of s/uint64/uint8/ could
be a replacement of this patch, and I hope that'll work too for the script.
So we will still see a bunch of 0x30s but I assume it's ok.

> +
>          return self.data
>  
>  class VMSDFieldIntLE(VMSDFieldInt):
> -- 
> 2.35.3
> 

-- 
Peter Xu