From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42238) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ukl3X-0004EI-V2 for qemu-devel@nongnu.org; Thu, 06 Jun 2013 21:01:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ukl3X-0001rl-24 for qemu-devel@nongnu.org; Thu, 06 Jun 2013 21:01:27 -0400 Received: from mail-qe0-f44.google.com ([209.85.128.44]:42279) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ukl3W-0001rd-UR for qemu-devel@nongnu.org; Thu, 06 Jun 2013 21:01:26 -0400 Received: by mail-qe0-f44.google.com with SMTP id 6so2404786qeb.31 for ; Thu, 06 Jun 2013 18:01:26 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <51B130D8.50108@redhat.com> Date: Thu, 06 Jun 2013 21:01:12 -0400 From: Paolo Bonzini MIME-Version: 1.0 References: <1370371954-8479-1-git-send-email-pbonzini@redhat.com> <1370371954-8479-12-git-send-email-pbonzini@redhat.com> <20130604220328.GB30400@redhat.com> <51AE6CC0.4050808@redhat.com> <20130605045348.GA31253@redhat.com> <51AEED43.2080302@redhat.com> <20130605103214.GC31830@redhat.com> In-Reply-To: <20130605103214.GC31830@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org 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