[Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value

[Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value

[Luiz Capitulino]

Use sizeof(strucy virtio_balloon_config) instead.

Signed-off-by: Luiz capitulino <lcapitulino@redhat.com>
---

I have no idea which queue this should go through...

 hw/virtio/virtio-balloon.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index d9754db..a470a0b 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -263,7 +263,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
     config.num_pages = cpu_to_le32(dev->num_pages);
     config.actual = cpu_to_le32(dev->actual);
 
-    memcpy(config_data, &config, 8);
+    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
 }
 
 static void virtio_balloon_set_config(VirtIODevice *vdev,
@@ -272,7 +272,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
     struct virtio_balloon_config config;
     uint32_t oldactual = dev->actual;
-    memcpy(&config, config_data, 8);
+    memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
     dev->actual = le32_to_cpu(config.actual);
     if (dev->actual != oldactual) {
         qemu_balloon_changed(ram_size -
@@ -343,7 +343,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     VirtIOBalloon *s = VIRTIO_BALLOON(dev);
     int ret;
 
-    virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON, 8);
+    virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
+                sizeof(struct virtio_balloon_config));
 
     ret = qemu_add_balloon_handler(virtio_balloon_to_target,
                                    virtio_balloon_stat, s);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value

[Peter Crosthwaite]

On Fri, Jan 10, 2014 at 12:58 AM, Luiz Capitulino
<lcapitulino@redhat.com> wrote:
> Use sizeof(strucy virtio_balloon_config) instead.
>

"struct".

> Signed-off-by: Luiz capitulino <lcapitulino@redhat.com>

Otherwise:

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>
> I have no idea which queue this should go through...
>

Trivial?

Regards,
Peter

>  hw/virtio/virtio-balloon.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index d9754db..a470a0b 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -263,7 +263,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>      config.num_pages = cpu_to_le32(dev->num_pages);
>      config.actual = cpu_to_le32(dev->actual);
>
> -    memcpy(config_data, &config, 8);
> +    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
>  }
>
>  static void virtio_balloon_set_config(VirtIODevice *vdev,
> @@ -272,7 +272,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>      VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
>      struct virtio_balloon_config config;
>      uint32_t oldactual = dev->actual;
> -    memcpy(&config, config_data, 8);
> +    memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
>      dev->actual = le32_to_cpu(config.actual);
>      if (dev->actual != oldactual) {
>          qemu_balloon_changed(ram_size -
> @@ -343,7 +343,8 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>      int ret;
>
> -    virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON, 8);
> +    virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
> +                sizeof(struct virtio_balloon_config));
>
>      ret = qemu_add_balloon_handler(virtio_balloon_to_target,
>                                     virtio_balloon_stat, s);
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value

[Michael Tokarev]

09.01.2014 18:58, Luiz Capitulino wrote:
> Use sizeof(strucy virtio_balloon_config) instead.
> 
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -263,7 +263,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>      config.num_pages = cpu_to_le32(dev->num_pages);
>      config.actual = cpu_to_le32(dev->actual);
>  
> -    memcpy(config_data, &config, 8);
> +    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));

I'm not sure any of those is better than another.

This is a published guest <=> host interface, the config _must_ be 8 bytes
long and must contain 2 4-byte words in it.

We may use assert(sizeof(struct virtio_balloon_config) == 8) somewhere,
but to my taste it is a bit overkill.  No?

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value

[Luiz Capitulino]

On Tue, 14 Jan 2014 21:05:31 +0400
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 09.01.2014 18:58, Luiz Capitulino wrote:
> > Use sizeof(strucy virtio_balloon_config) instead.
> > 
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -263,7 +263,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >      config.num_pages = cpu_to_le32(dev->num_pages);
> >      config.actual = cpu_to_le32(dev->actual);
> >  
> > -    memcpy(config_data, &config, 8);
> > +    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
> 
> I'm not sure any of those is better than another.

No duplication, no risk of changing virtio_balloon_config and
forgetting about changing all the memcpys out there (which is
exactly what happened to me). This is also what the other
devices do.

> This is a published guest <=> host interface, the config _must_ be 8 bytes
> long and must contain 2 4-byte words in it.

That's not changed by this patch.

> We may use assert(sizeof(struct virtio_balloon_config) == 8) somewhere,
> but to my taste it is a bit overkill.  No?
> 
> Thanks,
> 
> /mjt
> 

Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value

[Michael S. Tsirkin]

On Tue, Jan 14, 2014 at 09:05:31PM +0400, Michael Tokarev wrote:
> 09.01.2014 18:58, Luiz Capitulino wrote:
> > Use sizeof(strucy virtio_balloon_config) instead.
> > 
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -263,7 +263,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >      config.num_pages = cpu_to_le32(dev->num_pages);
> >      config.actual = cpu_to_le32(dev->actual);
> >  
> > -    memcpy(config_data, &config, 8);
> > +    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
> 
> I'm not sure any of those is better than another.
> 
> This is a published guest <=> host interface, the config _must_ be 8 bytes
> long and must contain 2 4-byte words in it.

no, config can be extended in the future.
and hard coded constants are evil.

> 
> We may use assert(sizeof(struct virtio_balloon_config) == 8) somewhere,
> but to my taste it is a bit overkill.  No?

I agree assert like this would be overkill.

> Thanks,
> 
> /mjt
-- 
MST

Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value

[Michael Tokarev]

09.01.2014 18:58, Luiz Capitulino wrote:
> Use sizeof(strucy virtio_balloon_config) instead.

Thanks, applied to the trivial patches queue (with the spelling fix).

/mjt

Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value

[Luiz Capitulino]

On Wed, 15 Jan 2014 20:10:31 +0400
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 09.01.2014 18:58, Luiz Capitulino wrote:
> > Use sizeof(strucy virtio_balloon_config) instead.
> 
> Thanks, applied to the trivial patches queue (with the spelling fix).

Thanks Michael.