| Message ID | 20140109095816.409cd2a9@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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
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 >
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
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
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.
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);
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(-)