On 02/07/2018 16:05, Emanuele Giuseppe Esposito wrote: > @@ -152,6 +152,8 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator > *alloc) > > void qpci_free_pc(QPCIBus *bus) > { > + g_assert(bus); > + > QPCIBusPC *s = container_of(bus, QPCIBusPC, bus); > > g_free(s); > diff --git a/tests/sdhci-test.c b/tests/sdhci-test.c > index 1d825eb010..9b486b93bf 100644 > --- a/tests/sdhci-test.c > +++ b/tests/sdhci-test.c > @@ -209,7 +209,9 @@ static QSDHCI *machine_start(const struct sdhci_t *test) > > static void machine_stop(QSDHCI *s) > { > - qpci_free_pc(s->pci.bus); > + if (s->pci.bus) { > + qpci_free_pc(s->pci.bus); > + }
Sorry for chiming in just now. In general, freeing a NULL pointer is a fine thing to do in C. In your code you do QPCIBusPC *ret = g_new0(QPCIBusPC, 1); qpci_set_pc(ret, qts, alloc); return &ret->bus; But now &ret->bus can be inside the pointer. qpci_free_pc must therefore check for NULL before doing the container_of. It is debatable whether this change should go in QEMU before your code, or together with it. There are good arguments for both sides: - the container_of is assuming that QPCIBus is the first field of the struct, but that's a strange assumption: container_of usually is used to go from an interior pointer to an outside struct, and passing NULL to it is usually wrong - but, the struct _does_ have QPCIBus as the first field, so we can assume that if bus == NULL, s will be NULL too. And g_free(NULL) is okay. I suggest that you add the "if (!bus) { return; }" in your code, in the same patch that adds the field before QPCIBusPC. Thanks, Paolo