On Mon, Jul 22, 2013 at 03:29:46PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <m...@redhat.com> writes:
>
> > On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
> >> Signed-off-by: Andreas Färber <afaer...@suse.de>
> >> ---
> >> hw/pci-bridge/ioh3420.c | 23 ++++++++++-------------
> >> hw/pci-bridge/xio3130_downstream.c | 23 ++++++++++-------------
> >> hw/pci-bridge/xio3130_upstream.c | 15 +++++++--------
> >> hw/pci/pcie_port.c | 22 ++++++++++++++++++++++
> >> include/hw/pci/pcie_port.h | 14 ++++++++++++--
> >> 5 files changed, 61 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> >> index 728f658..83db054 100644
> >> --- a/hw/pci-bridge/ioh3420.c
> >> +++ b/hw/pci-bridge/ioh3420.c
> >> @@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
> >>
> >> static int ioh3420_initfn(PCIDevice *d)
> >> {
> >> - PCIBridge *br = PCI_BRIDGE(d);
> >> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >> - PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
> >> + PCIEPort *p = PCIE_PORT(d);
> >> + PCIESlot *s = PCIE_SLOT(d);
> >> int rc;
> >>
> >> rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
> >> @@ -148,9 +147,7 @@ err_bridge:
> >>
> >> static void ioh3420_exitfn(PCIDevice *d)
> >> {
> >> - PCIBridge *br = PCI_BRIDGE(d);
> >> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >> - PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
> >> + PCIESlot *s = PCIE_SLOT(d);
> >>
> >> pcie_aer_exit(d);
> >> pcie_chassis_del_slot(s);
> >> @@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
> >> multifunction,
> >> qdev_prop_set_uint16(qdev, "slot", slot);
> >> qdev_init_nofail(qdev);
> >>
> >> - return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
> >> + return PCIE_SLOT(d);
> >> }
> >>
> >> static const VMStateDescription vmstate_ioh3420 = {
> >> @@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
> >> .minimum_version_id_old = 1,
> >> .post_load = pcie_cap_slot_post_load,
> >> .fields = (VMStateField[]) {
> >> - VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
> >> - VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
> >> + VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
> >> + VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
> >> vmstate_pcie_aer_log, PCIEAERLog),
> >> VMSTATE_END_OF_LIST()
> >> }
> >> };
> >>
> >> static Property ioh3420_properties[] = {
> >> - DEFINE_PROP_UINT8("port", PCIESlot, port.port, 0),
> >> + DEFINE_PROP_UINT8("port", PCIEPort, port, 0),
> >> DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0),
> >> DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0),
> >> - DEFINE_PROP_UINT16("aer_log_max", PCIESlot,
> >> - port.br.parent_obj.exp.aer_log.log_max,
> >> + DEFINE_PROP_UINT16("aer_log_max", PCIDevice,
> >> + exp.aer_log.log_max,
> >> PCIE_AER_LOG_MAX_DEFAULT),
> >> DEFINE_PROP_END_OF_LIST(),
> >> };
> >
> >
> > This looks scary. This does a cast to different types
> > without any checks at all.
>
> What cast?
>
> VMstate takes a void *.
>
> One an object is cast to a void *, it's just as much as PCIESlot as it
> is a PCIEPort.
>
> That said, for consistency, I think having everything be relatively to
> *one* type for a Property list is pretty helpful.
>
> Expecting someone to know the type hierarchy by heart such that this
> doesn't look like a bug is too much IMHO.
>
> Regards,
>
> Anthony Liguori
Exactly.
