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

[Juan Quintela <quintela@redhat.com>]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/virtio-net.c |  148 ++++++++++++++++++++++++-------------------------------
>>  1 files changed, 64 insertions(+), 84 deletions(-)
>> 
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index 4434827..3a59449 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -703,6 +703,38 @@ static void virtio_net_tx_timer(void *opaque)
>>      virtio_net_flush_tx(n, n->tx_vq);
>>  }
>> 
>> +/* Restore an uint8_t from an uint32_t
>> +   This is a Big hack, but it is how the old state did it.
>> + */
>
> I do not see why this is such a big hack.
> u8 fits in u32. there might be many reasons
> to use u32 for savevm (e.g. to be forward
> compatible) and still use u8 at runtime
> (e.g. to save memory).

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.

> 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.

>> +
>> +static int get_uint8_from_uint32(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    uint8_t *v = pv;
>> +    *v = qemu_get_be32(f);
>> +    return 0;
>> +}
>> +
>> +static void put_unused(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    fprintf(stderr, "uint8_from_uint32 is used only for backwards compatibility.\n");
>> +    fprintf(stderr, "Never should be used to write a new state.\n");
>> +    exit(0);
>> +}
>> +
>> +static const VMStateInfo vmstate_hack_uint8_from_uint32 = {
>> +    .name = "uint8_from_uint32",
>> +    .get  = get_uint8_from_uint32,
>> +    .put  = put_unused,
>> +};
>> +
>> +#define VMSTATE_UINT8_HACK_TEST(_f, _s, _t)                           \
>> +    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_hack_uint8_from_uint32, uint8_t)
>
>
> So a different format for a different version.
> Why is this so bad?
> And IMO VMSTATE macros really should be able to cope with
> version range checks without open-coded
> predicates. These are common and we will have more
> of these.

if they are common, we can have a library of them.  That is not the
problem.  cope with version range checks is not so trivial (at least we
need to add another field to store the in,from).  And there are places
where that is not enough (think of the virtio check to know if it is
pci/msix).

>> +
>> +static bool between_4_and_8(void *opaque, int version_id)
>> +{
>> +    return version_id >= 4 && version_id < 8;
>> +}
>> +
>
> This is pretty ugly, isn't it?
> How about ..._V_RANGE() macros?

I am really thinking about remove the _V() version, and move all to
tests.  reason for that is that there are already too much arguments
that are integers.  If we add more, we end having something like:

VMSTATE_FOO(bar, FOOState, 1, 2, 3, 4)

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.

This proposal has the advantage that it removes the version parameter
and improves typechecking.

Later, Juan.