From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MBYKe-00068z-4B for qemu-devel@nongnu.org; Tue, 02 Jun 2009 14:03:28 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MBYKa-00063b-5g for qemu-devel@nongnu.org; Tue, 02 Jun 2009 14:03:27 -0400 Received: from [199.232.76.173] (port=37455 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MBYKZ-00063Q-P7 for qemu-devel@nongnu.org; Tue, 02 Jun 2009 14:03:23 -0400 Received: from smtp-out.google.com ([216.239.33.17]:29950) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MBYKX-0006L7-Pj for qemu-devel@nongnu.org; Tue, 02 Jun 2009 14:03:23 -0400 Received: from spaceape14.eur.corp.google.com (spaceape14.eur.corp.google.com [172.28.16.148]) by smtp-out.google.com with ESMTP id n52I3HdY018893 for ; Tue, 2 Jun 2009 19:03:17 +0100 Received: from fxm7 (fxm7.prod.google.com [10.184.13.7]) by spaceape14.eur.corp.google.com with ESMTP id n52I3FpD012194 for ; Tue, 2 Jun 2009 11:03:16 -0700 Received: by fxm7 with SMTP id 7so1986003fxm.43 for ; Tue, 02 Jun 2009 11:03:15 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20090602085803.GB26814@redhat.com> References: <200905290758.11551.jcd@tribudubois.net> <4A1FD6E2.9020006@redhat.com> <200905291407.26757.paul@codesourcery.com> <200905291917.10535.jseward@acm.org> <60cad3f0905291412m670c7a6cw45f9b51f3122ddfb@mail.gmail.com> <4A24D40E.2060704@redhat.com> <4A24D92D.4010704@codemonkey.ws> <20090602085803.GB26814@redhat.com> Date: Tue, 2 Jun 2009 20:03:14 +0200 Message-ID: <60cad3f0906021103u75c46acx70d3580f6165faea@mail.gmail.com> Subject: Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently From: David Turner Content-Type: multipart/alternative; boundary=001636c5a7e84fa5d6046b615b71 List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Kevin Wolf , qemu-devel@nongnu.org, Jean-Christophe Dubois , Paul Brook , Gerd Hoffmann --001636c5a7e84fa5d6046b615b71 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit On Tue, Jun 2, 2009 at 10:58 AM, Daniel P. Berrange wrote: > On Tue, Jun 02, 2009 at 02:47:57AM -0500, Anthony Liguori wrote: > > Gerd Hoffmann wrote: > > > On 05/29/09 23:12, David Turner wrote: > > >> I would even suggest providing helper macros to make the programmer's > > >> intent > > >> even more clear > > >> and less error-prone, as in: > > >> > > >> #define QEMU_NEW(ptr) (ptr) = > > >> qemu_alloc(sizeof(*(ptr))) > > >> #define QEMU_NEW_ARRAY(ptr,cnt) (ptr) = > > >> qemu_calloc((cnt),sizeof(*(ptr))) > > >> #define QEMU_RENEW_ARRAY(ptr,cnt) (ptr) = > > >> qemu_realloc((ptr),(cnt),sizeof(*(ptr))) > > >> #define QEMU_FREE_ARRAY(ptr) qemu_free(ptr) > > > > > > The idea to have allocators for arrays (and have them allow > > > zero-length arrays) is fine. I wouldn't create two macros for new and > > > renew array, you can just use usual realloc semantics (ptr == NULL -> > > > alloc). > > > > > > Also I don't like the syntax that much as you'll have the IMHO > > > non-intuitive code like this: > > > > > > QEMU_NEW_ARRAY(ptr, ...); > > > > > > instead of > > > > > > ptr = QEMU_NEW_ARRAY(...); > > > > > > then. I don't see another easy way to get the automagic sizeof(*ptr) > > > stuff done though. > > > > I've always liked glib's memory functions. It does OOM error handling > > and returns NULL when size == 0. > > If you look at the problems associated with malloc there are many common > programmer mistakes, of which failure to check for NULL is just one. > IMHO, if you're going to wrap malloc/calloc/etc, then you should aim > higher and try to address all the common problems. David's suggestion > helps address the problem incorrect sizing too, of which there was an > example on this list only last week with VncState/VncDisplasy mixup. > Other problems including forgetting to initialize memory, which can be > solved by using calloc for everything (though in QEMU's case this may > have too much overhead). Double free is another which can be protected > against by having the free function also NULL-ify the pointer being > freed. > Agreed, that's the thing I do; and it works really well in practice. > > Regards, > Daniel > -- > |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/:| > |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org:| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/:| > |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 > :| > --001636c5a7e84fa5d6046b615b71 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Tue, Jun 2, 2009 at 10:58 AM, Daniel = P. Berrange <be= rrange@redhat.com> wrote:
On Tue, Jun 02, 2009 at 02:47:57AM -0500,= Anthony Liguori wrote:
> Gerd Hoffmann wrote:
> > On 05/29/09 23:12, David Turner wrote:
> >> I would even suggest providing helper macros to make the prog= rammer's
> >> intent
> >> even more clear
> >> and less error-prone, as in:
> >>
> >> #define =A0QEMU_NEW(ptr) =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0(ptr) =3D
> >> qemu_alloc(sizeof(*(ptr)))
> >> #define =A0QEMU_NEW_ARRAY(ptr,cnt) =A0 (ptr) =3D
> >> qemu_calloc((cnt),sizeof(*(ptr)))
> >> #define =A0QEMU_RENEW_ARRAY(ptr,cnt) =A0(ptr) =3D
> >> qemu_realloc((ptr),(cnt),sizeof(*(ptr)))
> >> #define =A0QEMU_FREE_ARRAY(ptr) =A0 =A0 =A0 =A0qemu_free(ptr)=
> >
> > The idea to have allocators for arrays (and have them allow
> > zero-length arrays) is fine. =A0I wouldn't create two macros = for new and
> > renew array, you can just use usual realloc semantics (ptr =3D=3D= NULL ->
> > alloc).
> >
> > Also I don't like the syntax that much as you'll have the= IMHO
> > non-intuitive code like this:
> >
> > =A0 QEMU_NEW_ARRAY(ptr, ...);
> >
> > instead of
> >
> > =A0 ptr =3D QEMU_NEW_ARRAY(...);
> >
> > then. =A0I don't see another easy way to get the automagic si= zeof(*ptr)
> > stuff done though.
>
> I've always liked glib's memory functions. =A0It does OOM erro= r handling
> and returns NULL when size =3D=3D 0.

If you look at the problems associated with malloc there are ma= ny common
programmer mistakes, of which failure to check for NULL is just one.
IMHO, if you're going to wrap malloc/calloc/etc, then you should aim higher and try to address all the common problems. =A0David's suggestio= n
helps address the problem incorrect sizing too, of which there was an
example on this list only last week with VncState/VncDisplasy mixup.
Other problems including forgetting to initialize memory, which can be
solved by using calloc for everything (though in QEMU's case this may have too much overhead). Double free is another which can be protected
against by having the free function also NULL-ify the pointer being
freed.

Agreed, that's the thing I d= o; and it works really well in practice.
=A0

Regards,
Daniel
--
|: Red Hat, Engineering, London =A0 -o- =A0 http://people.redhat.com/berrange/ :|=
|: http://libvirt.org = =A0-o- =A0http://virt= -manager.org =A0-o- =A0h= ttp://ovirt.org :|
|: http://autobuild.org<= /a> =A0 =A0 =A0 -o- =A0 =A0 =A0 =A0 http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 =A0-o- =A0F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9= 505 :|

--001636c5a7e84fa5d6046b615b71--