Peter Xu <pet...@redhat.com> writes:
> 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 <faro...@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.
Yes, actually I overlooked as well that it should match the size of the
data being handled in the get/put functions.
My comment below is about NULL -> 0x30 that I think should instead be
NULL -> 0x3030303030303030 so we have any chance of looking at this and
identifying it's a NULL pointer. When we write 0x30 it might become
confusing for people reading the scripts output that their stream has a
bunch of '0' in the place where pointers should be. If the MAGIC number
were more identifiable, I could change the script to output (null) or 0x0ULL.
We also don't really have the concept of a pointer, which I suspect
might be the real reason behind all this mess. So we'll see:
0x30
0x30
{
.some
.struct
.here
}
0x30
So all this patch was trying to do is document this situation somehow.
>
> 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
>>
