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.
