Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.

[Igor Mammedov]

On Fri, 06 Dec 2013 18:29:58 +0100
Paolo Bonzini <address@hidden> wrote:

> Il 06/12/2013 18:03, Igor Mammedov ha scritto:
> > +    hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), 
> > "hotplug-device",
> > +                                                  &local_err));
> > +    if (error_is_set(&local_err)) {
> > +        goto error_exit;
> > +    }
> > +    if (hotplug_dev) {
> > +        HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > +
> > +        /* handler can differentiate between hotplug and when device is
> > +         * enabled during qemu machine creation by inspecting
> > +         * dev->hotplugged field. */
> > +        if (hdc->hotplug) {
> > +            hdc->hotplug(hotplug_dev, qdev, &local_err);
> > +            if (error_is_set(&local_err)) {
> > +                int r = pci_unregister_device(&pci_dev->qdev);
> > +                assert(!r);
> > +                goto error_exit;
> > +            }
> >          }
> >      }
> 
> > +
> > +    hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), 
> > "hotplug-device",
> > +                                                  &local_err));
> > +    if (error_is_set(&local_err)) {
> > +        goto error_exit;
> > +    }
> > +    if (hotplug_dev) {
> > +        HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > +
> > +        if (hdc->hot_unplug) {
> > +            hdc->hot_unplug(hotplug_dev, qdev, &local_err);
> > +            if (error_is_set(&local_err)) {
> > +                goto error_exit;
> > +            }
> > +        }
> > +    }
> 
> Please move the parts under the "if" to hotplug.c (something like
> hotplug_handler_plug and hotplug_handler_unplug).  Also, should this be
sure.

> moved up to generic code (e.g. bus_add_child/bus_remove_child)?
Current usage hints that it's more realize() related part. If handler
fails device might want to do same device specific failure handling
before returning from realize() with failure.
It will work in bus_add_child/bus_remove_child as well but it would
basically put handlers to nofail category.

> 
> A NULL link can be a no-op for coldplugged devices, an error for
> hotplugged devices, and an assertion failure for hot-unplug.
> 
> Paolo
>