Re: [Qemu-devel] [PATCH RFC] pci: call PCIDeviceClass->exit on its .realize failure

[Marcel Apfelbaum <marcel@redhat.com>]

On 09/14/2016 12:14 PM, Cao jin wrote:
> It cannot guarantee all pci devices will free the allocated resource in
> its .realize function on realize failure.
>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>
> I found not all the devices will free the allocated resources on .realize
> failure, and .exit function is the one who take responsibility to free all
> the resource. In theory, I think it should be PCIDeviceClass->exit who does
> the cleanup on realize failure, with appropriate check whether certain
> resources is allocated.
> It passed make check, but maybe need more confirmation, so, RFC.
>
>  hw/pci/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 24fae16..4b63a79 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1967,6 +1967,9 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>          if (local_err) {
>              error_propagate(errp, local_err);
>              do_pci_unregister_device(pci_dev);
> +            if (pc->exit) {
> +                pc->exit(pci_dev);
> +            }

Hi,

I think the call to pc->exit is not necessary here.

The call to pc->realize failed, so it is the callee (pc)
responsibility to clean the resources.

Please see in the same method:

      pci_add_option_rom(pci_dev, is_default_rom, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             pci_qdev_unrealize(DEVICE(pci_dev), NULL);

              ^^^^^^^^^^^^^^^^^^^

This time we call pci_qdev_unrealize (that will call pc->exit)
because pc->realize succeeded and it is now the caller responsibility
to clean the resources.


Have you found a specific scenario that causes problems?

Thanks,
Marcel

>              return;
>          }
>      }
>