[Qemu-devel] Re: [PATCH 33/41] virtio-net: port to vmstate

[Juan Quintela]

"Michael S. Tsirkin" <address@hidden> wrote:

...

>> but u32 don't fit in u8.  And we read u32 into u8.  I agree that writing
>> u8 into u32 is not a problem, the problem is the reverse.
>
> So don't read u32 into u8.
> Read data into u32, check that the value fits into u8, fail migration
> if it does not, assign value if it does.

Exactly what the code does.  That is one of the definitions of "hack" :)

>> > IMO we should teach infrastructure to cope
>> > wit this gracefully without littering
>> > code with _hack suffixes.
>> 
>> There is no way to get this one.  vmstate only knows about types.
>> You want vmstate to know that:
>> - sometimes writing u32 in u8 is one error
>> - othertimes writing u32 in u8 is correct, because we should have using
>>   u8 in the 1st place.
>> 
>> vmstate only has to look at:
>> type of field and function used to send/receive field. No code analisys
>> to know how many values are used of that type, etc.
>> 
>> What code is doing here is not valid in general.  It is valid in this
>> particular case, that is the reason why it is a _hack.
>> 
>> Later, Juan.
>
> Assume that you want to make format forward compatible.  If you want to
> support large values in the future, you reserve 4 bytes in the format.
> This makes more sense than playing with versions, and in my book, this
> is a good practice, not a hack.  This should also not force you to store
> values at runtime as a 32 bit integers.

If you want 32bits, use 32bits.  No tricks, no games, all right.  If you
want to read 8bit into 32 bits, that is doable.  If you want to store
32bits into 8bit -> unsafe in general.  If you want to do that, you get
a big RED sing, you can call it "UNSAFE", "HACK", whatever.
Typechecking is lost.

>> Once that we are there, moving one integer to the wrong place is too
>> easy.  In the other hand, it is trivial to have already defined
>> functions that are:
>> v1
>> v2
>> v3
>> v1_8
>> ....
>> And for that, I can typecheck that you are not putting integers in the
>> place of versions.
>> 
>> technically, there is easy to add _RANGE() macros.  My problem is that
>> each time that we add another integer parameter, we make the interfaz
>> more difficult to use.
>
> I agree this is a problem.  Another option is to use structures
> initializing any extra fields outside the macro.
>
>       {
>               .min_version = 1,
>               .max_version = 2,
>               VMSTATE_FOO(bar, FOOState),
>       },

That is another option.  I would preffer the test functions, because as
we need them anyways, that makes things easier.  vmstate_field was
pretty small at the beggining, now it is starting to bloat.

Later, Juan.