| Message ID | 20210923121153.23754-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | Improve consistency of bus init function names | expand |
On 9/23/21 14:11, Peter Maydell wrote: > Currently we have a bit of a mishmash of different function > names for bus creation. There are two basic patterns: you > can have a function that allocates and returns a new bus > object; or you can have a function that takes a pointer to > a bus object and initializes it in-place. We have to some > extent a convention for those: the allocate-and-return > function is 'foo_new()', and the 'init in-place' function > is 'foo_init()'. However many of our bus creation functions > don't follow that; some use 'foo_new' vs 'foo_new_inplace'; > some use 'foo_new' for the in-place init version; and > the bottom level qbus functions are 'qbus_create' vs > 'qbus_create_inplace'. This series tries to bring at least > scsi, ipack, pci, ide, and qbus into line with the > _new-vs-_init naming convention. > > The other issue with bus creation functions is that some > of them take a 'name' argument which can be NULL, and some > do not. Generally "pass in a specific name" should be the > rare case, but our API design here is easy to misuse, and > so a lot of callsites (especially for i2c, sd, ssi) pass > in names when they should not. Untangling that mess is > going to be tricky (see other thread for more), but as > a first step, this series proposes a split between > foo_bus_new() and foo_bus_new_named() where the latter > takes a name parameter and the former does not. I do > this only for scsi (and implicitly ide, whose ide_bus_new > function already doesn't take a name argument) for the > moment, as the other bus types have more of a mess of > "pass name when they should not" callsites, so I didn't > want to put in too much work before finding out if we > had agreement on this as a naming convention. > > There are definitely more buses that can be cleaned up > to follow the init vs new convention, but this series is > already touching 70 files and trying to do every bus in > one series seems like a recipe for merge conflicts. > So this seemed like enough to be going on with... > > thanks > -- PMM > > Peter Maydell (6): > scsi: Replace scsi_bus_new() with scsi_bus_init(), > scsi_bus_init_named() > ipack: Rename ipack_bus_new_inplace() to ipack_bus_init() > pci: Rename pci_root_bus_new_inplace() to pci_root_bus_init() > qbus: Rename qbus_create_inplace() to qbus_init() > qbus: Rename qbus_create() to qbus_new() > ide: Rename ide_bus_new() to ide_bus_init() Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Thu, Sep 23, 2021 at 01:11:47PM +0100, Peter Maydell wrote: > Currently we have a bit of a mishmash of different function > names for bus creation. There are two basic patterns: you > can have a function that allocates and returns a new bus > object; or you can have a function that takes a pointer to > a bus object and initializes it in-place. We have to some > extent a convention for those: the allocate-and-return > function is 'foo_new()', and the 'init in-place' function > is 'foo_init()'. However many of our bus creation functions > don't follow that; some use 'foo_new' vs 'foo_new_inplace'; > some use 'foo_new' for the in-place init version; and > the bottom level qbus functions are 'qbus_create' vs > 'qbus_create_inplace'. This series tries to bring at least > scsi, ipack, pci, ide, and qbus into line with the > _new-vs-_init naming convention. > > The other issue with bus creation functions is that some > of them take a 'name' argument which can be NULL, and some > do not. Generally "pass in a specific name" should be the > rare case, but our API design here is easy to misuse, and > so a lot of callsites (especially for i2c, sd, ssi) pass > in names when they should not. Untangling that mess is > going to be tricky (see other thread for more), but as > a first step, this series proposes a split between > foo_bus_new() and foo_bus_new_named() where the latter > takes a name parameter and the former does not. I do > this only for scsi (and implicitly ide, whose ide_bus_new > function already doesn't take a name argument) for the > moment, as the other bus types have more of a mess of > "pass name when they should not" callsites, so I didn't > want to put in too much work before finding out if we > had agreement on this as a naming convention. > > There are definitely more buses that can be cleaned up > to follow the init vs new convention, but this series is > already touching 70 files and trying to do every bus in > one series seems like a recipe for merge conflicts. > So this seemed like enough to be going on with... > > thanks > -- PMM pci: Reviewed-by: Michael S. Tsirkin <mst@redhat.com> feel free to merge. > Peter Maydell (6): > scsi: Replace scsi_bus_new() with scsi_bus_init(), > scsi_bus_init_named() > ipack: Rename ipack_bus_new_inplace() to ipack_bus_init() > pci: Rename pci_root_bus_new_inplace() to pci_root_bus_init() > qbus: Rename qbus_create_inplace() to qbus_init() > qbus: Rename qbus_create() to qbus_new() > ide: Rename ide_bus_new() to ide_bus_init() > > include/hw/ide/internal.h | 4 ++-- > include/hw/ipack/ipack.h | 8 ++++---- > include/hw/pci/pci.h | 10 +++++----- > include/hw/qdev-core.h | 6 +++--- > include/hw/scsi/scsi.h | 30 ++++++++++++++++++++++++++++-- > hw/audio/intel-hda.c | 2 +- > hw/block/fdc.c | 2 +- > hw/block/swim.c | 3 +-- > hw/char/virtio-serial-bus.c | 4 ++-- > hw/core/bus.c | 13 +++++++------ > hw/core/sysbus.c | 10 ++++++---- > hw/gpio/bcm2835_gpio.c | 3 +-- > hw/hyperv/vmbus.c | 2 +- > hw/i2c/core.c | 2 +- > hw/ide/ahci.c | 2 +- > hw/ide/cmd646.c | 2 +- > hw/ide/isa.c | 2 +- > hw/ide/macio.c | 2 +- > hw/ide/microdrive.c | 2 +- > hw/ide/mmio.c | 2 +- > hw/ide/piix.c | 2 +- > hw/ide/qdev.c | 4 ++-- > hw/ide/sii3112.c | 2 +- > hw/ide/via.c | 2 +- > hw/ipack/ipack.c | 10 +++++----- > hw/ipack/tpci200.c | 4 ++-- > hw/isa/isa-bus.c | 2 +- > hw/misc/auxbus.c | 2 +- > hw/misc/mac_via.c | 4 ++-- > hw/misc/macio/cuda.c | 4 ++-- > hw/misc/macio/macio.c | 4 ++-- > hw/misc/macio/pmu.c | 4 ++-- > hw/nubus/mac-nubus-bridge.c | 2 +- > hw/nvme/ctrl.c | 4 ++-- > hw/nvme/subsys.c | 3 +-- > hw/pci-host/raven.c | 4 ++-- > hw/pci-host/versatile.c | 6 +++--- > hw/pci/pci.c | 30 +++++++++++++++--------------- > hw/pci/pci_bridge.c | 4 ++-- > hw/ppc/spapr_vio.c | 2 +- > hw/s390x/ap-bridge.c | 2 +- > hw/s390x/css-bridge.c | 2 +- > hw/s390x/event-facility.c | 4 ++-- > hw/s390x/s390-pci-bus.c | 2 +- > hw/s390x/virtio-ccw.c | 3 +-- > hw/scsi/esp-pci.c | 2 +- > hw/scsi/esp.c | 2 +- > hw/scsi/lsi53c895a.c | 2 +- > hw/scsi/megasas.c | 3 +-- > hw/scsi/mptsas.c | 2 +- > hw/scsi/scsi-bus.c | 6 +++--- > hw/scsi/spapr_vscsi.c | 3 +-- > hw/scsi/virtio-scsi.c | 4 ++-- > hw/scsi/vmw_pvscsi.c | 3 +-- > hw/sd/allwinner-sdhost.c | 4 ++-- > hw/sd/bcm2835_sdhost.c | 4 ++-- > hw/sd/pl181.c | 3 +-- > hw/sd/pxa2xx_mmci.c | 4 ++-- > hw/sd/sdhci.c | 3 +-- > hw/sd/ssi-sd.c | 3 +-- > hw/ssi/ssi.c | 2 +- > hw/usb/bus.c | 2 +- > hw/usb/dev-smartcard-reader.c | 3 +-- > hw/usb/dev-storage-bot.c | 3 +-- > hw/usb/dev-storage-classic.c | 4 ++-- > hw/usb/dev-uas.c | 3 +-- > hw/virtio/virtio-mmio.c | 3 +-- > hw/virtio/virtio-pci.c | 3 +-- > hw/xen/xen-bus.c | 2 +- > hw/xen/xen-legacy-backend.c | 2 +- > 70 files changed, 156 insertions(+), 142 deletions(-) > > -- > 2.20.1
On Thu, 23 Sept 2021 at 13:11, Peter Maydell <peter.maydell@linaro.org> wrote: > > Currently we have a bit of a mishmash of different function > names for bus creation. There are two basic patterns: you > can have a function that allocates and returns a new bus > object; or you can have a function that takes a pointer to > a bus object and initializes it in-place. We have to some > extent a convention for those: the allocate-and-return > function is 'foo_new()', and the 'init in-place' function > is 'foo_init()'. However many of our bus creation functions > don't follow that; some use 'foo_new' vs 'foo_new_inplace'; > some use 'foo_new' for the in-place init version; and > the bottom level qbus functions are 'qbus_create' vs > 'qbus_create_inplace'. This series tries to bring at least > scsi, ipack, pci, ide, and qbus into line with the > _new-vs-_init naming convention. Thanks all for the reviews/acks. I'm going to take this via target-arm.next. -- PMM
Currently we have a bit of a mishmash of different function names for bus creation. There are two basic patterns: you can have a function that allocates and returns a new bus object; or you can have a function that takes a pointer to a bus object and initializes it in-place. We have to some extent a convention for those: the allocate-and-return function is 'foo_new()', and the 'init in-place' function is 'foo_init()'. However many of our bus creation functions don't follow that; some use 'foo_new' vs 'foo_new_inplace'; some use 'foo_new' for the in-place init version; and the bottom level qbus functions are 'qbus_create' vs 'qbus_create_inplace'. This series tries to bring at least scsi, ipack, pci, ide, and qbus into line with the _new-vs-_init naming convention. The other issue with bus creation functions is that some of them take a 'name' argument which can be NULL, and some do not. Generally "pass in a specific name" should be the rare case, but our API design here is easy to misuse, and so a lot of callsites (especially for i2c, sd, ssi) pass in names when they should not. Untangling that mess is going to be tricky (see other thread for more), but as a first step, this series proposes a split between foo_bus_new() and foo_bus_new_named() where the latter takes a name parameter and the former does not. I do this only for scsi (and implicitly ide, whose ide_bus_new function already doesn't take a name argument) for the moment, as the other bus types have more of a mess of "pass name when they should not" callsites, so I didn't want to put in too much work before finding out if we had agreement on this as a naming convention. There are definitely more buses that can be cleaned up to follow the init vs new convention, but this series is already touching 70 files and trying to do every bus in one series seems like a recipe for merge conflicts. So this seemed like enough to be going on with... thanks -- PMM Peter Maydell (6): scsi: Replace scsi_bus_new() with scsi_bus_init(), scsi_bus_init_named() ipack: Rename ipack_bus_new_inplace() to ipack_bus_init() pci: Rename pci_root_bus_new_inplace() to pci_root_bus_init() qbus: Rename qbus_create_inplace() to qbus_init() qbus: Rename qbus_create() to qbus_new() ide: Rename ide_bus_new() to ide_bus_init() include/hw/ide/internal.h | 4 ++-- include/hw/ipack/ipack.h | 8 ++++---- include/hw/pci/pci.h | 10 +++++----- include/hw/qdev-core.h | 6 +++--- include/hw/scsi/scsi.h | 30 ++++++++++++++++++++++++++++-- hw/audio/intel-hda.c | 2 +- hw/block/fdc.c | 2 +- hw/block/swim.c | 3 +-- hw/char/virtio-serial-bus.c | 4 ++-- hw/core/bus.c | 13 +++++++------ hw/core/sysbus.c | 10 ++++++---- hw/gpio/bcm2835_gpio.c | 3 +-- hw/hyperv/vmbus.c | 2 +- hw/i2c/core.c | 2 +- hw/ide/ahci.c | 2 +- hw/ide/cmd646.c | 2 +- hw/ide/isa.c | 2 +- hw/ide/macio.c | 2 +- hw/ide/microdrive.c | 2 +- hw/ide/mmio.c | 2 +- hw/ide/piix.c | 2 +- hw/ide/qdev.c | 4 ++-- hw/ide/sii3112.c | 2 +- hw/ide/via.c | 2 +- hw/ipack/ipack.c | 10 +++++----- hw/ipack/tpci200.c | 4 ++-- hw/isa/isa-bus.c | 2 +- hw/misc/auxbus.c | 2 +- hw/misc/mac_via.c | 4 ++-- hw/misc/macio/cuda.c | 4 ++-- hw/misc/macio/macio.c | 4 ++-- hw/misc/macio/pmu.c | 4 ++-- hw/nubus/mac-nubus-bridge.c | 2 +- hw/nvme/ctrl.c | 4 ++-- hw/nvme/subsys.c | 3 +-- hw/pci-host/raven.c | 4 ++-- hw/pci-host/versatile.c | 6 +++--- hw/pci/pci.c | 30 +++++++++++++++--------------- hw/pci/pci_bridge.c | 4 ++-- hw/ppc/spapr_vio.c | 2 +- hw/s390x/ap-bridge.c | 2 +- hw/s390x/css-bridge.c | 2 +- hw/s390x/event-facility.c | 4 ++-- hw/s390x/s390-pci-bus.c | 2 +- hw/s390x/virtio-ccw.c | 3 +-- hw/scsi/esp-pci.c | 2 +- hw/scsi/esp.c | 2 +- hw/scsi/lsi53c895a.c | 2 +- hw/scsi/megasas.c | 3 +-- hw/scsi/mptsas.c | 2 +- hw/scsi/scsi-bus.c | 6 +++--- hw/scsi/spapr_vscsi.c | 3 +-- hw/scsi/virtio-scsi.c | 4 ++-- hw/scsi/vmw_pvscsi.c | 3 +-- hw/sd/allwinner-sdhost.c | 4 ++-- hw/sd/bcm2835_sdhost.c | 4 ++-- hw/sd/pl181.c | 3 +-- hw/sd/pxa2xx_mmci.c | 4 ++-- hw/sd/sdhci.c | 3 +-- hw/sd/ssi-sd.c | 3 +-- hw/ssi/ssi.c | 2 +- hw/usb/bus.c | 2 +- hw/usb/dev-smartcard-reader.c | 3 +-- hw/usb/dev-storage-bot.c | 3 +-- hw/usb/dev-storage-classic.c | 4 ++-- hw/usb/dev-uas.c | 3 +-- hw/virtio/virtio-mmio.c | 3 +-- hw/virtio/virtio-pci.c | 3 +-- hw/xen/xen-bus.c | 2 +- hw/xen/xen-legacy-backend.c | 2 +- 70 files changed, 156 insertions(+), 142 deletions(-) -- 2.20.1