Re: [Qemu-devel] [PATCH 15/38] ivshmem: Failed realize() can leave migration blocker behind

[Marc-André Lureau]

Hi

On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
> If pci_ivshmem_realize() fails after it created its migration blocker,
> the blocker is left in place.  Fix that by creating it last.
>
> Likewise, if it fails after it called fifo8_create(), it leaks fifo
> memory.  Fix that the same way.
>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---

Make sense, I didn't fully get that "realize" was suppose to handle
failure properly.

Btw, why do you introduce a new err variable? I guess that's easier to
deal with, perhaps in a following patch.

other than that
Reviewed-by: Marc-André Lureau <address@hidden>



>  hw/misc/ivshmem.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index eb53d9a..1392426 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -824,6 +824,7 @@ static void ivshmem_write_config(PCIDevice *pdev, 
> uint32_t address,
>  static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>  {
>      IVShmemState *s = IVSHMEM(dev);
> +    Error *err = NULL;
>      uint8_t *pci_conf;
>      uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>          PCI_BASE_ADDRESS_MEM_PREFETCH;
> @@ -855,8 +856,6 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>          s->ivshmem_size = size;
>      }
>
> -    fifo8_create(&s->incoming_fifo, sizeof(int64_t));
> -
>      /* IRQFD requires MSI */
>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD) &&
>          !ivshmem_has_feature(s, IVSHMEM_MSI)) {
> @@ -878,12 +877,6 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>          s->role_val = IVSHMEM_MASTER; /* default */
>      }
>
> -    if (s->role_val == IVSHMEM_PEER) {
> -        error_setg(&s->migration_blocker,
> -                   "Migration is disabled when using feature 'peer mode' in 
> device 'ivshmem'");
> -        migrate_add_blocker(s->migration_blocker);
> -    }
> -
>      pci_conf = dev->config;
>      pci_conf[PCI_COMMAND] = PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
>
> @@ -962,7 +955,19 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>              return;
>          }
>
> -        create_shared_memory_BAR(s, fd, attr, errp);
> +        create_shared_memory_BAR(s, fd, attr, &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +    }
> +
> +    fifo8_create(&s->incoming_fifo, sizeof(int64_t));
> +
> +    if (s->role_val == IVSHMEM_PEER) {
> +        error_setg(&s->migration_blocker,
> +                   "Migration is disabled when using feature 'peer mode' in 
> device 'ivshmem'");
> +        migrate_add_blocker(s->migration_blocker);
>      }
>  }
>
> --
> 2.4.3
>
>



-- 
Marc-André Lureau