From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NH35R-0001t8-Vx for qemu-devel@nongnu.org; Sat, 05 Dec 2009 17:26:46 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NH35M-0001ou-Bk for qemu-devel@nongnu.org; Sat, 05 Dec 2009 17:26:44 -0500 Received: from [199.232.76.173] (port=40845 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NH35M-0001oo-9n for qemu-devel@nongnu.org; Sat, 05 Dec 2009 17:26:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:28175) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NH35M-0007Vy-1E for qemu-devel@nongnu.org; Sat, 05 Dec 2009 17:26:40 -0500 Message-ID: <4B1ADE14.2070809@redhat.com> Date: Sun, 06 Dec 2009 00:26:28 +0200 From: Avi Kivity MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Permit zero-sized qemu_malloc() & friends References: <4B193DA5.6040507@codemonkey.ws> <4B1A9359.8080305@redhat.com> <4B1A9811.8020108@codemonkey.ws> <4B1A9AF9.8000107@redhat.com> <4B1A9E39.2030602@codemonkey.ws> <4B1AA110.8030600@redhat.com> <4B1AC96B.7060007@codemonkey.ws> In-Reply-To: <4B1AC96B.7060007@codemonkey.ws> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org, Paul Brook , Markus Armbruster On 12/05/2009 10:58 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> When we see a lengthy and error prone idiom we usually provide a >> wrapper. That wrapper is qemu_malloc(). If you like, don't see it >> as a fixed malloc(), but as qemu's way of allocating memory which is >> totally independent from malloc(). > > We constantly get patches with qemu_malloc() with a NULL check. Then > we tell people to remove the NULL check. It feels very weird to ask > people to remove error handling. You prefer to explain to them how to do error handling correctly? > > I can understand the argument that getting OOM right is very difficult > but it's not impossible. There are 755 calls to malloc in the code. And practically every syscall can return ENOMEM, including the innocuous KVM_RUN ioctl(). It's going to be pretty close to impossible to recover from malloc() failure, and impossible to recover from KVM_RUN failure (except by retrying, which you can assume the kernel already has). All for something which never happens. I propose that fixing OOM handling is going to introduce some errors into the non-error paths, and many errors into the error return paths, for zero benefit. > >>> >>> However, this is all personal preference and I'd rather focus my >>> energy on things that have true functional impact. Markus raised a >>> valid functional problem with the current implementation and I >>> proposed a solution that would address that functional problem. I'd >>> rather see the discussion focus on the merits of that solution than >>> revisiting whether ANSI got the semantics of malloc() correct in the >>> standards definition. >>> >> >> Unless ANSI has a say on qemu_malloc(), I think it's worthwhile to >> get that right rather than wrapping every array caller with useless >> tests. > > If you're concerned about array allocation, introduce an array > allocation function. Honestly, there's very little reason to open > code array allocation/manipulation at all. We should either be using > a list type or if we really need to, we should introduce a vector type. A NEW(type) and ARRAY_NEW(type, count) marcros would improve type safety and plug a dormant buffer overflow due to multiplication overflow, yes. Even qemu_calloc() would be an improvement. But having qemu_malloc() not fix the zero length array case which we know we have is irresponsible, IMO. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic.