* [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value
@ 2014-01-09 14:58 Luiz Capitulino
2014-01-09 15:41 ` Peter Crosthwaite
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Luiz Capitulino @ 2014-01-09 14:58 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value
2014-01-09 14:58 [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value Luiz Capitulino
@ 2014-01-09 15:41 ` Peter Crosthwaite
2014-01-14 17:05 ` Michael Tokarev
2014-01-15 16:10 ` Michael Tokarev
2 siblings, 0 replies; 7+ messages in thread
From: Peter Crosthwaite @ 2014-01-09 15:41 UTC (permalink / raw)
To: Luiz Capitulino, qemu-trivial; +Cc: qemu-devel, Michael S. Tsirkin
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
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value
2014-01-09 14:58 [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value Luiz Capitulino
2014-01-09 15:41 ` Peter Crosthwaite
@ 2014-01-14 17:05 ` Michael Tokarev
2014-01-14 18:29 ` Luiz Capitulino
2014-01-14 20:05 ` Michael S. Tsirkin
2014-01-15 16:10 ` Michael Tokarev
2 siblings, 2 replies; 7+ messages in thread
From: Michael Tokarev @ 2014-01-14 17:05 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-trivial, Peter Crosthwaite, qemu-devel, mst
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value
2014-01-14 17:05 ` Michael Tokarev
@ 2014-01-14 18:29 ` Luiz Capitulino
2014-01-14 20:05 ` Michael S. Tsirkin
1 sibling, 0 replies; 7+ messages in thread
From: Luiz Capitulino @ 2014-01-14 18:29 UTC (permalink / raw)
To: Michael Tokarev; +Cc: qemu-trivial, Peter Crosthwaite, qemu-devel, mst
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value
2014-01-14 17:05 ` Michael Tokarev
2014-01-14 18:29 ` Luiz Capitulino
@ 2014-01-14 20:05 ` Michael S. Tsirkin
1 sibling, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2014-01-14 20:05 UTC (permalink / raw)
To: Michael Tokarev
Cc: qemu-trivial, Peter Crosthwaite, qemu-devel, Luiz Capitulino
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value
2014-01-09 14:58 [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value Luiz Capitulino
2014-01-09 15:41 ` Peter Crosthwaite
2014-01-14 17:05 ` Michael Tokarev
@ 2014-01-15 16:10 ` Michael Tokarev
2014-01-15 16:36 ` Luiz Capitulino
2 siblings, 1 reply; 7+ messages in thread
From: Michael Tokarev @ 2014-01-15 16:10 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel, mst
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-01-15 16:36 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-09 14:58 [Qemu-devel] [PATCH] virtio-balloon: don't hardcode config size value Luiz Capitulino
2014-01-09 15:41 ` Peter Crosthwaite
2014-01-14 17:05 ` Michael Tokarev
2014-01-14 18:29 ` Luiz Capitulino
2014-01-14 20:05 ` Michael S. Tsirkin
2014-01-15 16:10 ` Michael Tokarev
2014-01-15 16:36 ` Luiz Capitulino
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).