Il 05/06/2013 06:32, Michael S. Tsirkin ha scritto: > On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote: >> Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto: >>> On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote: >>>> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto: >>>>>>> + if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) { >>>>>>> + msix_free(dev); >>>>>>> + } >>>>>>> + >>>>>>> dev->msix_table = g_malloc0(table_size); >>>>>>> dev->msix_pba = g_malloc0(pba_size); >>>>>>> dev->msix_entry_used = g_malloc0(nentries * sizeof >>>>>>> *dev->msix_entry_used); >>>>> Wow msix_init calls msix_free, and not on error path? >>>>> What's going on here? >>>> >>>> I wasn't too sure that you could get here only with NULL >>>> msix_table/pba/entry_used and wanted to protect against leaks. I'll >>>> change it to an assertion. >>> >>> I don't think we should require users allocate all memory with g_malloc0. >>> So no assertion either. >> >> Assertion that is is NULL, followed by g_malloc0? > > No because who sets it to NULL the first time? > msix_init just started.
When an object is created, it is all-zeroed. >>> If there's a leak there was always a leak >> >> No, there wasn't because msix_uninit would have freed the memory. That is, >> >> msix_init >> msix_uninit >> msix_init >> msix_uninit >> >> had no leak. Instead, now msix_free is going to be called just once, >> right before freeing the object itself: >> >> msix_init >> msix_uninit >> msix_init *** >> msix_uninit >> msix_free >> >> and will have a leak at ***. > > Yes. And this looks completely sane from outside, > so this is a bad API. > The way to fix it is not with asserts in code, we need a good API: > alloc/free init/uninit ... Can't, because table_size/pba_size is not available at init time (e.g. for VFIO not until the host BARs are processed). What about using g_realloc + memset? Paolo