[29/41] virtio-net: in_use and first_multi only handle unsigned values

Submitted by Juan Quintela on Dec. 2, 2009, 12:04 p.m.

Details

Message ID ecf99588b0fbdc1540e03dec4a65c66493b7ab61.1259754427.git.quintela@redhat.com
State New
Headers show

Commit Message

Juan Quintela Dec. 2, 2009, 12:04 p.m.
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(-)

Comments

Michael S. Tsirkin Dec. 2, 2009, 2:52 p.m.
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
Juan Quintela Dec. 2, 2009, 6:30 p.m.
"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.
Michael S. Tsirkin Dec. 2, 2009, 6:32 p.m.
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.
Juan Quintela Dec. 2, 2009, 6:55 p.m.
"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.
Michael S. Tsirkin Dec. 2, 2009, 6:58 p.m.
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.

Patch hide | download patch | download mbox

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