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

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 08:18:40PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > On Wed, Dec 02, 2009 at 01:04:31PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <address@hidden>
> >> ---
> >>  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.

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.

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

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

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),
        },

> 
> This proposal has the advantage that it removes the version parameter
> and improves typechecking.
> 
> Later, Juan.