From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NGbLp-0006to-OF for qemu-devel@nongnu.org; Fri, 04 Dec 2009 11:49:49 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NGbLl-0006kj-2r for qemu-devel@nongnu.org; Fri, 04 Dec 2009 11:49:49 -0500 Received: from [199.232.76.173] (port=53743 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NGbLl-0006kW-01 for qemu-devel@nongnu.org; Fri, 04 Dec 2009 11:49:45 -0500 Received: from mail-qy0-f194.google.com ([209.85.221.194]:36365) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NGbLk-0006MF-QX for qemu-devel@nongnu.org; Fri, 04 Dec 2009 11:49:44 -0500 Received: by qyk32 with SMTP id 32so1081769qyk.4 for ; Fri, 04 Dec 2009 08:49:44 -0800 (PST) Message-ID: <4B193DA5.6040507@codemonkey.ws> Date: Fri, 04 Dec 2009 10:49:41 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Permit zero-sized qemu_malloc() & friends References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Paul Brook Markus Armbruster wrote: > Commit a7d27b53 made zero-sized allocations a fatal error, deviating > from ISO C's malloc() & friends. Revert that, but take care never to > return a null pointer, like malloc() & friends may do (it's > implementation defined), because that's another source of bugs. > > Rationale: while zero-sized allocations might occasionally be a sign of > something going wrong, they can also be perfectly legitimate. The > change broke such legitimate uses. We've found and "fixed" at least one > of them already (commit eb0b64f7, also reverted by this patch), and > another one just popped up: the change broke qcow2 images with virtual > disk size zero, i.e. images that don't hold real data but only VM state > of snapshots. > I still believe that it is poor practice to pass size==0 to *malloc(). I think actively discouraging this in qemu is a good thing because it's a broken idiom. That said, we've had this change in for a while and it's painfully obviously we haven't eliminated all of these instances in qemu. Knowing that we still have occurrences of this and actively exit()'ing when they happen is pretty much suicidal in a production environment. It's not a bug, it's just a poor practice. Here's what I propose. I think we should introduce a CONFIG_DEBUG_ZERO_MALLOC. When this is set, we should assert(size!=0). Otherwise, we should return malloc(1) as Markus' patch does below. Using the same rules as we follow for -Werror, we should enable CONFIG_DEBUG_ZERO_MALLOC for the development tree and disable it for any releases. This helps us make qemu better during development while not unnecessarily causing us harm in a production environment. What happens here long term I think remains to be seen. But for right now, I think this is an important change to make for 0.12. > Signed-off-by: Markus Armbruster > --- > block/qcow2-snapshot.c | 5 ----- > qemu-malloc.c | 14 ++------------ > 2 files changed, 2 insertions(+), 17 deletions(-) > > diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c > index 94cb838..e3b208c 100644 > --- a/block/qcow2-snapshot.c > +++ b/block/qcow2-snapshot.c > @@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) > QCowSnapshot *sn; > int i; > > - if (!s->nb_snapshots) { > - *psn_tab = NULL; > - return s->nb_snapshots; > - } > - > This does not belong here. Regards, Anthony Liguori