| Message ID | a512b7850e08f0839cd2403c7178d98c8253da20.1259754427.git.quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 02, 2009 at 01:04:04PM +0100, Juan Quintela wrote: > virtio_common_init() creates a struct with the right size, DO_UPCAST > is the appropiate thing here > > Signed-off-by: Juan Quintela <quintela@redhat.com> BTW why not container_of? That one does not require field to be at the beginning of structure. > --- > hw/virtio-balloon.c | 4 ++-- > hw/virtio-blk.c | 8 ++++---- > hw/virtio-console.c | 3 ++- > hw/virtio-net.c | 8 ++++---- > 4 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c > index 2310ab0..6f60fb1 100644 > --- a/hw/virtio-balloon.c > +++ b/hw/virtio-balloon.c > @@ -167,11 +167,11 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id) > VirtIODevice *virtio_balloon_init(DeviceState *dev) > { > VirtIOBalloon *s; > - > - s = (VirtIOBalloon *)virtio_common_init("virtio-balloon", > + VirtIODevice *vdev = virtio_common_init("virtio-balloon", > VIRTIO_ID_BALLOON, > 8, sizeof(VirtIOBalloon)); > > + s = DO_UPCAST(VirtIOBalloon, vdev, vdev); > s->vdev.get_config = virtio_balloon_get_config; > s->vdev.set_config = virtio_balloon_set_config; > s->vdev.get_features = virtio_balloon_get_features; > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index 39ebc37..918be74 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -487,11 +487,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo) > char *ps = (char *)drive_get_serial(dinfo->bdrv); > size_t size = strlen(ps) ? sizeof(struct virtio_blk_config) : > offsetof(struct virtio_blk_config, _blk_size); > + VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK, > + size, > + sizeof(VirtIOBlock)); > > - s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK, > - size, > - sizeof(VirtIOBlock)); > - > + s = DO_UPCAST(VirtIOBlock, vdev, vdev); > s->config_size = size; > s->vdev.get_config = virtio_blk_update_config; > s->vdev.get_features = virtio_blk_get_features; > diff --git a/hw/virtio-console.c b/hw/virtio-console.c > index 9f1a602..57f5e9d 100644 > --- a/hw/virtio-console.c > +++ b/hw/virtio-console.c > @@ -121,9 +121,10 @@ static int virtio_console_load(QEMUFile *f, void *opaque, int version_id) > VirtIODevice *virtio_console_init(DeviceState *dev) > { > VirtIOConsole *s; > - s = (VirtIOConsole *)virtio_common_init("virtio-console", > + VirtIODevice *vdev = virtio_common_init("virtio-console", > VIRTIO_ID_CONSOLE, > 0, sizeof(VirtIOConsole)); > + s = DO_UPCAST(VirtIOConsole, vdev, vdev); > s->vdev.get_features = virtio_console_get_features; > > s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input); > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index f518d78..1b8ce14 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -817,11 +817,11 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf) > { > VirtIONet *n; > static int virtio_net_id; > + VirtIODevice *vdev = virtio_common_init("virtio-net", VIRTIO_ID_NET, > + sizeof(struct virtio_net_config), > + sizeof(VirtIONet)); > > - n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET, > - sizeof(struct virtio_net_config), > - sizeof(VirtIONet)); > - > + n = DO_UPCAST(VirtIONet, vdev, vdev); > n->vdev.get_config = virtio_net_get_config; > n->vdev.set_config = virtio_net_set_config; > n->vdev.get_features = virtio_net_get_features; > -- > 1.6.5.2
On Wed, Dec 02, 2009 at 07:19:17PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 01:04:04PM +0100, Juan Quintela wrote: > >> virtio_common_init() creates a struct with the right size, DO_UPCAST > >> is the appropiate thing here > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > > > BTW why not container_of? That one does not require > > field to be at the beginning of structure. > > VirtIO devices (and PCIDevices) are declared in this way: > > typedef struct VirtIOBalloon > { > VirtIODevice vdev; > VirtQueue *ivq, *dvq; > uint32_t num_pages; > uint32_t actual; > } VirtIOBalloon; > > > I.e. the virtioDevice is always the 1st element, otherwise things don't > work. There are code that requires it to be the 1st element. I know. But I think we should slowly fix these assumptions, and not introduce more of them. IOW: don't use DO_UPCAST. > > Later, Juan.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 01:04:04PM +0100, Juan Quintela wrote: >> virtio_common_init() creates a struct with the right size, DO_UPCAST >> is the appropiate thing here >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > BTW why not container_of? That one does not require > field to be at the beginning of structure. VirtIO devices (and PCIDevices) are declared in this way: typedef struct VirtIOBalloon { VirtIODevice vdev; VirtQueue *ivq, *dvq; uint32_t num_pages; uint32_t actual; } VirtIOBalloon; I.e. the virtioDevice is always the 1st element, otherwise things don't work. There are code that requires it to be the 1st element. Later, Juan.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 07:19:17PM +0100, Juan Quintela wrote: >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> > On Wed, Dec 02, 2009 at 01:04:04PM +0100, Juan Quintela wrote: >> >> virtio_common_init() creates a struct with the right size, DO_UPCAST >> >> is the appropiate thing here >> >> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> > >> > BTW why not container_of? That one does not require >> > field to be at the beginning of structure. >> >> VirtIO devices (and PCIDevices) are declared in this way: >> >> typedef struct VirtIOBalloon >> { >> VirtIODevice vdev; >> VirtQueue *ivq, *dvq; >> uint32_t num_pages; >> uint32_t actual; >> } VirtIOBalloon; >> >> >> I.e. the virtioDevice is always the 1st element, otherwise things don't >> work. There are code that requires it to be the 1st element. > > I know. But I think we should slowly fix these assumptions, and not > introduce more of them. IOW: don't use DO_UPCAST. It is inherent in how to implement OOP in C. You want to use things as pci devices and as pci specific devices. Then you need to put the pci common fields at the beggining. Yes, for virtio devices they want to be pci and virtio devices, but neither pci or virtio nor qemu in general allow to be derived of two types (a.k.a. as multiple inheritance). I think that we should continue using DO_UPCAST() until there are some design that allows you to change that. This particular case "requires" that VirtioDevice is the 1st field of the struct. If you change the code enough to make container_of() work, doing the s/DO_UPCAST/container_of/ is going to be the less of your problems. Later, Juan.
On Wed, Dec 02, 2009 at 07:42:35PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 07:19:17PM +0100, Juan Quintela wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > On Wed, Dec 02, 2009 at 01:04:04PM +0100, Juan Quintela wrote: > >> >> virtio_common_init() creates a struct with the right size, DO_UPCAST > >> >> is the appropiate thing here > >> >> > >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> > > >> > BTW why not container_of? That one does not require > >> > field to be at the beginning of structure. > >> > >> VirtIO devices (and PCIDevices) are declared in this way: > >> > >> typedef struct VirtIOBalloon > >> { > >> VirtIODevice vdev; > >> VirtQueue *ivq, *dvq; > >> uint32_t num_pages; > >> uint32_t actual; > >> } VirtIOBalloon; > >> > >> > >> I.e. the virtioDevice is always the 1st element, otherwise things don't > >> work. There are code that requires it to be the 1st element. > > > > I know. But I think we should slowly fix these assumptions, and not > > introduce more of them. IOW: don't use DO_UPCAST. > > It is inherent in how to implement OOP in C. You want to use things as > pci devices and as pci specific devices. Then you need to put the pci > common fields at the beggining. Yes, for virtio devices they want to be > pci and virtio devices, but neither pci or virtio nor qemu in general > allow to be derived of two types (a.k.a. as multiple inheritance). > > I think that we should continue using DO_UPCAST() until there are some > design that allows you to change that. This particular case "requires" > that VirtioDevice is the 1st field of the struct. If you change the > code enough to make container_of() work, doing the > s/DO_UPCAST/container_of/ is going to be the less of your problems. > > Later, Juan. I don't understand. container_of is just more generic than DO_UPCAST. So why *ever* use DO_UPCAST? Let's get rid of it.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > I don't understand. > container_of is just more generic than DO_UPCAST. > So why *ever* use DO_UPCAST? Let's get rid of it. functions that use a PCIDevice and you pass FooState "require" that PCIDevice to be the 1st element in the struct. Notice that it is "required", not "would be nice" or similar. If that is not the way, things dont' work. In this particular case, it is also required that VirtIODevice to be the 1st element: VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, size_t config_size, size_t struct_size) { VirtIODevice *vdev; vdev = qemu_mallocz(struct_size); vdev->device_id = device_id; vdev->status = 0; vdev->isr = 0; vdev->queue_sel = 0; vdev->config_vector = VIRTIO_NO_VECTOR; vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); vdev->name = name; vdev->config_len = config_size; if (vdev->config_len) vdev->config = qemu_mallocz(config_size); else vdev->config = NULL; return vdev; } See how you create a device of size struct_size, but then you access it with vdev. If vdev is _not_ the 1st element of the struct, you have got corruption. DO_UPCAST() prevent you for having that error. container_of() would have leave you go around, and have a memory corruption not easy to fix. DO_UPCAST() macro was created just to avoid this kind of errors. Later, Juan.
On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > I don't understand. > > container_of is just more generic than DO_UPCAST. > > So why *ever* use DO_UPCAST? Let's get rid of it. > > functions that use a PCIDevice and you pass FooState "require" that > PCIDevice to be the 1st element in the struct. If these used container_of consistently, maybe we won't get this limitation. > > Notice that it is "required", not "would be nice" or similar. If that > is not the way, things dont' work. > > In this particular case, it is also > required that VirtIODevice to be the 1st element: > > VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, > size_t config_size, size_t struct_size) > { > VirtIODevice *vdev; > > vdev = qemu_mallocz(struct_size); > > vdev->device_id = device_id; > vdev->status = 0; > vdev->isr = 0; > vdev->queue_sel = 0; > vdev->config_vector = VIRTIO_NO_VECTOR; > vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); > > vdev->name = name; > vdev->config_len = config_size; > if (vdev->config_len) > vdev->config = qemu_mallocz(config_size); > else > vdev->config = NULL; > > return vdev; > } > > See how you create a device of size struct_size, but then you access it > with vdev. If vdev is _not_ the 1st element of the struct, you have got > corruption. A cleaner solution IMO would be to have callers allocate the memory and pass VirtIODevice * to virtio_common_init. > DO_UPCAST() prevent you for having that error. If we want to assert specific structure layout, this should be a compile-time check. There's no reason to do this check every time at a random place where DO_UPCAST is called. > container_of() would have leave you go around, and have a memory > corruption not easy to fix. > > DO_UPCAST() macro was created just to avoid this kind of errors. > > Later, Juan.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote: >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >> > I don't understand. >> > container_of is just more generic than DO_UPCAST. >> > So why *ever* use DO_UPCAST? Let's get rid of it. .... >> See how you create a device of size struct_size, but then you access it >> with vdev. If vdev is _not_ the 1st element of the struct, you have got >> corruption. > > A cleaner solution IMO would be to have callers allocate the memory > and pass VirtIODevice * to virtio_common_init. Been there, asked for that. Basically qdev + passing initialized memory = nono >> DO_UPCAST() prevent you for having that error. > > > If we want to assert specific structure layout, this > should be a compile-time check. There's no > reason to do this check every time at a random place where > DO_UPCAST is called. See DO_UPCAST() definition :) It is a compile time check. It just do cpp magic to be sure that things are right. DO_UPCAST() == (cast *) with some typechecking. >> container_of() would have leave you go around, and have a memory >> corruption not easy to fix. >> >> DO_UPCAST() macro was created just to avoid this kind of errors. >> >> Later, Juan.
On Thu, Dec 03, 2009 at 12:56:57PM +0100, Juan Quintela wrote: > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >> > I don't understand. > >> > container_of is just more generic than DO_UPCAST. > >> > So why *ever* use DO_UPCAST? Let's get rid of it. > > .... > > >> See how you create a device of size struct_size, but then you access it > >> with vdev. If vdev is _not_ the 1st element of the struct, you have got > >> corruption. > > > > A cleaner solution IMO would be to have callers allocate the memory > > and pass VirtIODevice * to virtio_common_init. > > Been there, asked for that. Basically qdev + passing initialized memory > = nono It does not have to be initialized. > >> DO_UPCAST() prevent you for having that error. > > > > > > If we want to assert specific structure layout, this > > should be a compile-time check. There's no > > reason to do this check every time at a random place where > > DO_UPCAST is called. > > See DO_UPCAST() definition :) > > > It is a compile time check. It just do cpp magic to be sure that things > are right. DO_UPCAST() == (cast *) with some typechecking. Yes, but it gives the error on the wrong place. So it is only good as long as you do not change the code. In the example you give with virtio_init_common, there's no UPCAST to document the layout assumption *in the place where assumption is made*. On the other hand, DO_UPCAST is scattered all over the code in places which could work fine without any assumptions. Let's assume you want to change layout. You find all places with DO_UPCAST, fix them, and it will compile-but not work. Let's assume you change all code that makes layout assumptions to not make them: DO_UPCAST will still be hang around in other code. > >> container_of() would have leave you go around, and have a memory > >> corruption not easy to fix. > >> > >> DO_UPCAST() macro was created just to avoid this kind of errors. > >> > >> Later, Juan.
"Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Dec 03, 2009 at 12:56:57PM +0100, Juan Quintela wrote: >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> > On Wed, Dec 02, 2009 at 08:03:22PM +0100, Juan Quintela wrote: >> >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >> >> >> > I don't understand. >> >> > container_of is just more generic than DO_UPCAST. >> >> > So why *ever* use DO_UPCAST? Let's get rid of it. >> >> .... >> >> >> See how you create a device of size struct_size, but then you access it >> >> with vdev. If vdev is _not_ the 1st element of the struct, you have got >> >> corruption. >> > >> > A cleaner solution IMO would be to have callers allocate the memory >> > and pass VirtIODevice * to virtio_common_init. >> >> Been there, asked for that. Basically qdev + passing initialized memory >> = nono > > It does not have to be initialized. sorry, already allocated memory. basically -device foo requires that qdev creates foo and then it call foo_init(). You can see in the mail archive that I wanted a qdev_create_here() function, to do exactly what you wanted. It has problems with getting -device working realiablely. >> >> DO_UPCAST() prevent you for having that error. >> > >> > >> > If we want to assert specific structure layout, this >> > should be a compile-time check. There's no >> > reason to do this check every time at a random place where >> > DO_UPCAST is called. >> >> See DO_UPCAST() definition :) >> >> >> It is a compile time check. It just do cpp magic to be sure that things >> are right. DO_UPCAST() == (cast *) with some typechecking. > > Yes, but it gives the error on the wrong place. So it is > only good as long as you do not change the code. No, it gives it in the right place. Where you do a cast from PCIDevice -> FOOState or when you do a cast from VirtioDevice -> VirtIOFooDevice. Really it is a "smarter" cast than just do (VirtioFooDevice *). > In the example you give with virtio_init_common, there's no UPCAST > to document the layout assumption *in the place where assumption is made*. > On the other hand, DO_UPCAST is scattered all over the code > in places which could work fine without any assumptions. > > Let's assume you want to change layout. You find all places > with DO_UPCAST, fix them, and it will compile-but not work. > Let's assume you change all code that makes layout assumptions > to not make them: DO_UPCAST will still be hang around in other > code. Not requiring DO_UPCAST() mean changing how qdev work. If you have counted the Gerd qdev patches over the last months, you will see that it is an almost "infinity" ammount of work. I don't see why you want to change that. In this specific example, DO_UPCAST() is needed. It is required that VirtIODevice is the 1st field of any other structure. I really don't see why you want to change that. Later, Juan.
On 12/03/2009 02:55 PM, Juan Quintela wrote: > sorry, already allocated memory. basically -device foo > requires that qdev creates foo and then it call foo_init(). > Alternatives: - foo_init() allocates Foo and calls qdev_init() (how C++ works, but cumbersome here) - we put the offset of DeviceState as well as the size into DeviceInfo (a macro can help) I agree the hidden assumptions about member placement are bad.
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c index 2310ab0..6f60fb1 100644 --- a/hw/virtio-balloon.c +++ b/hw/virtio-balloon.c @@ -167,11 +167,11 @@ static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id) VirtIODevice *virtio_balloon_init(DeviceState *dev) { VirtIOBalloon *s; - - s = (VirtIOBalloon *)virtio_common_init("virtio-balloon", + VirtIODevice *vdev = virtio_common_init("virtio-balloon", VIRTIO_ID_BALLOON, 8, sizeof(VirtIOBalloon)); + s = DO_UPCAST(VirtIOBalloon, vdev, vdev); s->vdev.get_config = virtio_balloon_get_config; s->vdev.set_config = virtio_balloon_set_config; s->vdev.get_features = virtio_balloon_get_features; diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 39ebc37..918be74 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -487,11 +487,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, DriveInfo *dinfo) char *ps = (char *)drive_get_serial(dinfo->bdrv); size_t size = strlen(ps) ? sizeof(struct virtio_blk_config) : offsetof(struct virtio_blk_config, _blk_size); + VirtIODevice *vdev = virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK, + size, + sizeof(VirtIOBlock)); - s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK, - size, - sizeof(VirtIOBlock)); - + s = DO_UPCAST(VirtIOBlock, vdev, vdev); s->config_size = size; s->vdev.get_config = virtio_blk_update_config; s->vdev.get_features = virtio_blk_get_features; diff --git a/hw/virtio-console.c b/hw/virtio-console.c index 9f1a602..57f5e9d 100644 --- a/hw/virtio-console.c +++ b/hw/virtio-console.c @@ -121,9 +121,10 @@ static int virtio_console_load(QEMUFile *f, void *opaque, int version_id) VirtIODevice *virtio_console_init(DeviceState *dev) { VirtIOConsole *s; - s = (VirtIOConsole *)virtio_common_init("virtio-console", + VirtIODevice *vdev = virtio_common_init("virtio-console", VIRTIO_ID_CONSOLE, 0, sizeof(VirtIOConsole)); + s = DO_UPCAST(VirtIOConsole, vdev, vdev); s->vdev.get_features = virtio_console_get_features; s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input); diff --git a/hw/virtio-net.c b/hw/virtio-net.c index f518d78..1b8ce14 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -817,11 +817,11 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf) { VirtIONet *n; static int virtio_net_id; + VirtIODevice *vdev = virtio_common_init("virtio-net", VIRTIO_ID_NET, + sizeof(struct virtio_net_config), + sizeof(VirtIONet)); - n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET, - sizeof(struct virtio_net_config), - sizeof(VirtIONet)); - + n = DO_UPCAST(VirtIONet, vdev, vdev); n->vdev.get_config = virtio_net_get_config; n->vdev.set_config = virtio_net_set_config; n->vdev.get_features = virtio_net_get_features;
virtio_common_init() creates a struct with the right size, DO_UPCAST is the appropiate thing here Signed-off-by: Juan Quintela <quintela@redhat.com> --- hw/virtio-balloon.c | 4 ++-- hw/virtio-blk.c | 8 ++++---- hw/virtio-console.c | 3 ++- hw/virtio-net.c | 8 ++++---- 4 files changed, 12 insertions(+), 11 deletions(-)