Skip to contentSkip to conversation

virtio-balloon: don't hardcode config size value

PATCH Applied Luiz Capitulino 4 participants 7 messages Jan 9, 2014 – Jan 15, 2014

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 virtio-balloon: don't hardcode config size value

Use sizeof(strucy virtio_balloon_config) instead.

---

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

 hw/virtio/virtio-balloon.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
hw/virtio/virtio-balloon.c +4 -3
index d9754db..a470a0b 100644
@@ -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);
Signature · 1 line
1.8.1.4
Reviewed-by on v1 1/1
#1 lore raw
Quoted 2 lines · Luiz Capitulino

Use sizeof(strucy virtio_balloon_config) instead.

"struct".

Quoted 1 line

Otherwise:

Quoted 4 lines

---

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

Trivial?

Regards,
Peter

Quoted 39 lines
 hw/virtio/virtio-balloon.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
hw/virtio/virtio-balloon.c +4 -4
index d9754db..a470a0b 100644
@@ -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

#2 lore raw
In reply to Luiz Capitulino

hw/virtio/virtio-balloon.c static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) · line 266

Quoted 10 lines · Luiz Capitulino

Use sizeof(strucy virtio_balloon_config) instead.

hw/virtio/virtio-balloon.c +1 -1
@@ -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

#3 lore raw
Quoted 9 lines · On Tue, 14 Jan 2014 21:05:31 +0400 Michael Tokarev <[email protected]> wrote:
Quoted 8 lines · Luiz Capitulino

Use sizeof(strucy virtio_balloon_config) instead.

hw/virtio/virtio-balloon.c +0 -0
@@ -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);
 
Quoted 4 lines

hw/virtio/virtio-balloon.c static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) · line 266

Quoted 2 lines
hw/virtio/virtio-balloon.c +1 -1
-    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.

Quoted 2 lines

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.

Quoted 7 lines

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

Thanks,

/mjt

#4 lore raw
In reply to Michael Tokarev
Quoted 9 lines · Michael Tokarev
Quoted 8 lines · Luiz Capitulino

Use sizeof(strucy virtio_balloon_config) instead.

hw/virtio/virtio-balloon.c +0 -0
@@ -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);
 
Quoted 7 lines

hw/virtio/virtio-balloon.c static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data) · line 266

Quoted 2 lines
hw/virtio/virtio-balloon.c +1 -1
-    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.

Quoted 3 lines

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.

Quoted 3 lines

Thanks,

/mjt

Signature · 1 line
MST
#6 lore raw
Quoted 4 lines · On Wed, 15 Jan 2014 20:10:31 +0400 Michael Tokarev <[email protected]> wrote:
Quoted 1 line · Luiz Capitulino

Use sizeof(strucy virtio_balloon_config) instead.

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

Thanks Michael.