On Tue, Sep 17, 2013 at 11:58:12AM +0200, Paolo Bonzini wrote: > Il 17/09/2013 11:24, Michael S. Tsirkin ha scritto: > >> @@ -630,15 +630,21 @@ int shpc_bar_size(PCIDevice *d) > >> void shpc_cleanup(PCIDevice *d, MemoryRegion *bar) > >> { > >> SHPCDevice *shpc = d->shpc; > >> + /* TODO: cleanup config space changes? */ > >> d->cap_present &= ~QEMU_PCI_CAP_SHPC; > >> memory_region_del_subregion(bar, &shpc->mmio); > >> - /* TODO: cleanup config space changes? */ > >> +} > >> + > >> +void shpc_free(PCIDevice *d) > >> +{ > >> + SHPCDevice *shpc = d->shpc; > >> g_free(shpc->config); > >> g_free(shpc->cmask); > >> g_free(shpc->wmask); > >> g_free(shpc->w1cmask); > >> memory_region_destroy(&shpc->mmio); > >> g_free(shpc); > >> + d->shpc = NULL; > > I dislike having random dangling pointers.
I don't care much (though there's something to be said for not initializing memory you don't expect to be used - it's a compact way to document no one will look at it). But if this is changed I'd like to do such changes in a separate patch. > But it's not that bad if I > move the clearing of QEMU_PCI_CAP_SHPC from shpc_cleanup to shpc_free. > > Paolo Yea.