| Message ID | 1456771254-17511-16-git-send-email-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com> > --- 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 <marcandre.lureau@redhat.com> > 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 <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <armbru@redhat.com> 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 <armbru@redhat.com> >> --- > > 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 <marcandre.lureau@redhat.com> > > > >> 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 >> >>
Hi On Wed, Mar 2, 2016 at 10:54 AM, Markus Armbruster <armbru@redhat.com> wrote: >>> - 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? Got it, thanks.
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); } }
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 <armbru@redhat.com> --- hw/misc/ivshmem.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)