| Message ID | ecf99588b0fbdc1540e03dec4a65c66493b7ab61.1259754427.git.quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 02, 2009 at 01:04:27PM +0100, Juan Quintela wrote: > And they were saved/loaded as unsigned already I'm not sure how does on save/load a value as unsigned. Could you please provide motivation for this and similar changes? Not that it matters very much, but int is slightly cleaner than uint32_t for something that is only 1 or 0. > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > hw/virtio-net.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index 05cc67f..589ea80 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -50,8 +50,8 @@ typedef struct VirtIONet > uint8_t nouni; > uint8_t nobcast; > struct { > - int in_use; > - int first_multi; > + uint32_t in_use; > + uint32_t first_multi; > uint8_t multi_overflow; > uint8_t uni_overflow; > uint8_t *macs; > @@ -715,7 +715,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque) > qemu_put_be16s(f, &n->status); > qemu_put_8s(f, &n->promisc); > qemu_put_8s(f, &n->allmulti); > - qemu_put_be32(f, n->mac_table.in_use); > + qemu_put_be32s(f, &n->mac_table.in_use); > qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN); > qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3); > qemu_put_be32(f, n->has_vnet_hdr); > @@ -756,7 +756,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) > } > > if (version_id >= 5) { > - n->mac_table.in_use = qemu_get_be32(f); > + qemu_get_be32s(f, &n->mac_table.in_use); > qemu_get_buffer(f, n->mac_table.macs, > n->mac_table.in_use * ETH_ALEN); > } > -- > 1.6.5.2
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 01:04:27PM +0100, Juan Quintela wrote: >> And they were saved/loaded as unsigned already > > I'm not sure how does on save/load a value as unsigned. > Could you please provide motivation for this > and similar changes? > Not that it matters very much, but int is slightly cleaner > than uint32_t for something that is only 1 or 0. > >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> hw/virtio-net.c | 8 ++++---- >> 1 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c >> index 05cc67f..589ea80 100644 >> --- a/hw/virtio-net.c >> +++ b/hw/virtio-net.c >> @@ -50,8 +50,8 @@ typedef struct VirtIONet >> uint8_t nouni; >> uint8_t nobcast; >> struct { >> - int in_use; >> - int first_multi; >> + uint32_t in_use; >> + uint32_t first_multi; >> uint8_t multi_overflow; >> uint8_t uni_overflow; >> uint8_t *macs; >> @@ -715,7 +715,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque) >> qemu_put_be16s(f, &n->status); >> qemu_put_8s(f, &n->promisc); >> qemu_put_8s(f, &n->allmulti); >> - qemu_put_be32(f, n->mac_table.in_use); This is why I hate this function. >> + qemu_put_be32s(f, &n->mac_table.in_use); This is the right one. We can change things to be int32_t if that makes more sense (they were sent as uint32). vmstate checks that the type of the value that you sent and the function that you use for sending match. In this case, it was sending a int32_t with the function to send uint32_t. In this particular case it didn't matter (value is 0/1). But vmstate don't know what cases matter/don't matter. It just test _always_ that you are using the right function for your type. If you think that it is better to change the type of the value to int32_t and change the functions, that is also ok with me. What I care is that type of function and field are the same. Later, Juan.
On Wed, Dec 02, 2009 at 07:30:18PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 01:04:27PM +0100, Juan Quintela wrote: > >> And they were saved/loaded as unsigned already > > > > I'm not sure how does on save/load a value as unsigned. > > Could you please provide motivation for this > > and similar changes? > > Not that it matters very much, but int is slightly cleaner > > than uint32_t for something that is only 1 or 0. > > > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> --- > >> hw/virtio-net.c | 8 ++++---- > >> 1 files changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c > >> index 05cc67f..589ea80 100644 > >> --- a/hw/virtio-net.c > >> +++ b/hw/virtio-net.c > >> @@ -50,8 +50,8 @@ typedef struct VirtIONet > >> uint8_t nouni; > >> uint8_t nobcast; > >> struct { > >> - int in_use; > >> - int first_multi; > >> + uint32_t in_use; > >> + uint32_t first_multi; > >> uint8_t multi_overflow; > >> uint8_t uni_overflow; > >> uint8_t *macs; > >> @@ -715,7 +715,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque) > >> qemu_put_be16s(f, &n->status); > >> qemu_put_8s(f, &n->promisc); > >> qemu_put_8s(f, &n->allmulti); > >> - qemu_put_be32(f, n->mac_table.in_use); > > This is why I hate this function. > > >> + qemu_put_be32s(f, &n->mac_table.in_use); > > This is the right one. > > We can change things to be int32_t if that makes more sense (they were sent > as uint32). > > vmstate checks that the type of the value that you sent and the function > that you use for sending match. In this case, it was sending a int32_t > with the function to send uint32_t. In this particular case it didn't > matter (value is 0/1). But vmstate don't know what cases matter/don't > matter. It just test _always_ that you are using the right function for > your type. > > If you think that it is better to change the type of the value to > int32_t and change the functions, that is also ok with me. > What I care is that type of function and field are the same. > > Later, Juan. int is a better type than int32 here. Please make the save/load functions match. If you like, convert it to bool.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 07:30:18PM +0100, Juan Quintela wrote: >> We can change things to be int32_t if that makes more sense (they were sent >> as uint32). >> >> vmstate checks that the type of the value that you sent and the function >> that you use for sending match. In this case, it was sending a int32_t >> with the function to send uint32_t. In this particular case it didn't >> matter (value is 0/1). But vmstate don't know what cases matter/don't >> matter. It just test _always_ that you are using the right function for >> your type. >> >> If you think that it is better to change the type of the value to >> int32_t and change the functions, that is also ok with me. >> What I care is that type of function and field are the same. >> >> Later, Juan. > > int is a better type than int32 here. will switch to int (I really hope that int32_t == int in all architectures that we are interested in) > Please make the save/load functions match. > If you like, convert it to bool. bool is only 1 byte, not four. I wasn't the one using 4 bytes for a bool. Later, Juan.
On Wed, Dec 02, 2009 at 07:55:51PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 07:30:18PM +0100, Juan Quintela wrote: > > >> We can change things to be int32_t if that makes more sense (they were sent > >> as uint32). > >> > >> vmstate checks that the type of the value that you sent and the function > >> that you use for sending match. In this case, it was sending a int32_t > >> with the function to send uint32_t. In this particular case it didn't > >> matter (value is 0/1). But vmstate don't know what cases matter/don't > >> matter. It just test _always_ that you are using the right function for > >> your type. > >> > >> If you think that it is better to change the type of the value to > >> int32_t and change the functions, that is also ok with me. > >> What I care is that type of function and field are the same. > >> > >> Later, Juan. > > > > int is a better type than int32 here. > > will switch to int (I really hope that int32_t == int in all > architectures that we are interested in) > > > Please make the save/load functions match. > > If you like, convert it to bool. > > bool is only 1 byte, not four. I wasn't the one using 4 bytes for a bool. > > Later, Juan. So just range check it. The fact we leave some space in format does not mean we must waste memory at runtime.
diff --git a/hw/virtio-net.c b/hw/virtio-net.c index 05cc67f..589ea80 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -50,8 +50,8 @@ typedef struct VirtIONet uint8_t nouni; uint8_t nobcast; struct { - int in_use; - int first_multi; + uint32_t in_use; + uint32_t first_multi; uint8_t multi_overflow; uint8_t uni_overflow; uint8_t *macs; @@ -715,7 +715,7 @@ static void virtio_net_save(QEMUFile *f, void *opaque) qemu_put_be16s(f, &n->status); qemu_put_8s(f, &n->promisc); qemu_put_8s(f, &n->allmulti); - qemu_put_be32(f, n->mac_table.in_use); + qemu_put_be32s(f, &n->mac_table.in_use); qemu_put_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN); qemu_put_buffer(f, n->vlans, MAX_VLAN >> 3); qemu_put_be32(f, n->has_vnet_hdr); @@ -756,7 +756,7 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) } if (version_id >= 5) { - n->mac_table.in_use = qemu_get_be32(f); + qemu_get_be32s(f, &n->mac_table.in_use); qemu_get_buffer(f, n->mac_table.macs, n->mac_table.in_use * ETH_ALEN); }
And they were saved/loaded as unsigned already Signed-off-by: Juan Quintela <quintela@redhat.com> --- hw/virtio-net.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)