From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MA9Ol-0007ow-BY for qemu-devel@nongnu.org; Fri, 29 May 2009 17:13:55 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MA9Og-0007oM-Io for qemu-devel@nongnu.org; Fri, 29 May 2009 17:13:54 -0400 Received: from [199.232.76.173] (port=32887 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MA9Og-0007oJ-FK for qemu-devel@nongnu.org; Fri, 29 May 2009 17:13:50 -0400 Received: from smtp-out.google.com ([216.239.33.17]:53817) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MA9Of-0003gw-Sj for qemu-devel@nongnu.org; Fri, 29 May 2009 17:13:50 -0400 Received: from wpaz29.hot.corp.google.com (wpaz29.hot.corp.google.com [172.24.198.93]) by smtp-out.google.com with ESMTP id n4TLDjSZ002774 for ; Fri, 29 May 2009 22:13:45 +0100 Received: from bwz8 (bwz8.prod.google.com [10.188.26.8]) by wpaz29.hot.corp.google.com with ESMTP id n4TLDL1L024433 for ; Fri, 29 May 2009 14:13:43 -0700 Received: by bwz8 with SMTP id 8so7041118bwz.41 for ; Fri, 29 May 2009 14:13:43 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <60cad3f0905291412m670c7a6cw45f9b51f3122ddfb@mail.gmail.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> Date: Fri, 29 May 2009 23:13:42 +0200 Message-ID: <60cad3f0905291413o17170e7dm5448d9653fabefc9@mail.gmail.com> Subject: Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently From: David Turner Content-Type: multipart/alternative; boundary=001636c5b3ff2285e3046b138da6 List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Julian Seward Cc: Kevin Wolf , Paul Brook , Gerd Hoffmann , qemu-devel@nongnu.org, Jean-Christophe Dubois --001636c5b3ff2285e3046b138da6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Aargh, I was confused, I meant: - qemu_calloc(count, itemsize) would abort for 'itemsize == 0', but return NULL for 'count == 0' sorry about that.. On Fri, May 29, 2009 at 11:12 PM, David Turner wrote: > > > On Fri, May 29, 2009 at 7:17 PM, Julian Seward wrote: > >> >> +1 for that. Code that relies on malloc(0) doing any specific thing >> is basically bad news when it comes to portability, robustness >> and understandability. Better to have qemu_malloc(0) abort, put up with >> a couple of days of the trunk aborting, until these uses are fixed. >> I'd be surprised if there were many cases anyway. >> > > I think there are two conflicting goals here: > > - On one hand, people are suggesting to abort on malloc(0) because it is > the sign of buggy code, > and would help spot it as soon as possible, before any damage is done. > > - On the other hand, some people object that this assumption is sometimes > wrong (e.g. with > arrays), where having malloc(0) returning NULL or anything else is > totally appropriate. > > Would it make sense to separate the two issues with something like the > following: > > - qemu_malloc(0) aborts and print a panic message > - qemu_calloc(count, itemsize) would return NULL for 'itemsize == 0', but > would abort for 'count == 0' > - array-allocating/parsing code MUST use qemu_calloc() exclusively. And > calloc() can also perform trivial > integer multiplication overflow checks too. > > 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) > > (yes, qemu_realloc() would take 3 parameters). > > Any direct use of malloc()/qemu_malloc() in source code would be suspicious > and could > easily spotted to check it. > > >> >> J >> >> >> > --001636c5b3ff2285e3046b138da6 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Aargh, I was confused, I meant:

- qemu_calloc(count, itemsize) would= abort for 'itemsize =3D=3D 0', but return NULL for 'count =3D= =3D 0'

sorry about that..

On F= ri, May 29, 2009 at 11:12 PM, David Turner <digit@google.com> wrote:


On Fri, May 29, 2009 at 7:17 PM, Julian= Seward <jseward@acm.org> wrote:

+1 for that. =A0Code that relies on malloc(0) doing any specific thin= g
is basically bad news when it comes to portability, robustness
and understandability. =A0Better to have qemu_malloc(0) abort, put up with<= br> a couple of days of the trunk aborting, until these uses are fixed.
I'd be surprised if there were many cases anyway.

I think there ar= e two conflicting goals here:

- On one hand, people are suggesting t= o abort on malloc(0) because it is the sign of buggy code,
=A0 and would= help spot it as soon as possible, before any damage is done.

- On the other hand, some people object that this assumption is sometim= es wrong (e.g. with
=A0 arrays), where having malloc(0) returning NULL o= r anything else is totally appropriate.

Would it make sense to separ= ate the two issues with something like the following:

- qemu_malloc(0) aborts and print a panic message
- qemu_calloc(coun= t, itemsize) would return NULL for 'itemsize =3D=3D 0', but would a= bort for 'count =3D=3D 0'
- array-allocating/parsing code MUST u= se qemu_calloc() exclusively. And calloc() can also perform trivial
integer multiplication overflow checks too.

I would even suggest pro= viding helper macros to make the programmer's intent even more clearand less error-prone, as in:

#define=A0 QEMU_NEW(ptr)=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (ptr) =3D qemu_alloc(sizeof(*= (ptr)))
#define=A0 QEMU_NEW_ARRAY(ptr,cnt)=A0=A0 (ptr) =3D qemu_calloc((cnt),sizeof= (*(ptr)))
#define=A0 QEMU_RENEW_ARRAY(ptr,cnt)=A0 (ptr) =3D qemu_realloc= ((ptr),(cnt),sizeof(*(ptr)))
#define=A0 QEMU_FREE_ARRAY(ptr)=A0 =A0=A0= =A0=A0=A0 qemu_free(ptr)

(yes, qemu_realloc() would take 3 parameters).

Any direct use of= malloc()/qemu_malloc() in source code would be suspicious and could
eas= ily spotted to check it.
=A0

J




--001636c5b3ff2285e3046b138da6--