Re: [Qemu-devel] [PATCH] Permit zero-sized qemu_malloc() & friends

[Avi Kivity]

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.