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

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> 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.

See explanation inline.

> 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;
>> +        }

Before my patch, passing errp to create_shared_memory_BAR() was fine,
because it was the last thing the function does.

Now, it isn't: we must bypass the rest of the function on error.

All clear now?

>> +    }
>> +
>> +    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
>>
>>