Re: [Qemu-devel] [PATCH 5/5] Convert remaining calls to g_malloc(sizeof(type)) to g_new()

[Stuart Brady <sdb@zubnet.me.uk>]

On Fri, Oct 21, 2011 at 09:37:02AM +0200, Paolo Bonzini wrote:
> On 10/21/2011 02:26 AM, Stuart Brady wrote:
> >>>  They all look okay, perhaps the include path you passed to
> >>>  Coccinelle is incomplete?
> >Ah, good point!  I'm not sure what include dirs are needed, though...
> >anyone have any advice?
> >
> >Blue Swirl, I gather you're one of the few other people to have used
> >Coccinelle with Qemu's source...
> 
> I played a bit yesterday and it turns out that Coccinelle is a bit
> limited WRT handling headers, because they are very expensive.  I
> used "-I . -I +build -I hw" but it didn't help much.
> 
> Stuart/Blue, do you have a macro file?  Mine was simply "#define
> coroutine_fn".

I didn't even have that, but Coccinelle didn't seem to mind...

It did occur to me that since a lot of Qemu's source is recompiled with
different macro definitions for different targets, we need to be really
careful about what we do regarding includes.  Hopefully the names of
types that are used won't vary between targets, though.

Submitting what Coccinelle could process successfully and fixing up the
rest manually seemed reasonable, but I'd like to be as confident as
possible of these changes.

BTW, I'd thought that noone would ever do E = (T *)g_malloc(sizeof(*E)),
but from looking hw/blizzard.c, hw/cbus.c and hw/nseries.c, it seems
that this isn't quite the case afterall!  I'll be sure to include this
in my second attempt, once QEMU 1.0 has been released.

One thing that did not occur to me is use of E = malloc(sizeof(*E1)) or
E = malloc(sizeof(T1)) where E is of type void *, but E1 or T1 is not
what was intended.

I'm also somewhat astonished to find that sizeof(void) and sizeof(*E)
where E is of type void * both compile!  It would probably make sense to
check for these.

Any remaining calls to g_malloc() would be then be reviewed to make sure
that they're all correct.

We could also perhaps search for places where free() is called on memory
that is allocated with g_malloc(), as g_free() should be used instead.

---

Some background on my thinking before sending the patch series:

(T *)g_malloc(sizeof(T)) can obviously be safely replaced with
g_new(T, 1) since that's what g_new(T, 1) expands to.

Replacing E = g_malloc(sizeof(*E)) with E = g_new(T, 1) adds a cast, but
the cast does not provide any extra safety, since sizeof(*T) is pretty
much certain to be the correct size (unless T = void *).  There seems
to be some agreement that this is more readable, though.

Replacing E = g_malloc(sizeof(T)) without a cast with E = g_new(T, 1)
effectively just adds a cast to T *, which might result in additional
compilation warnings (which are turned into errors) but should have no
other effect, so this should be perfectly safe.

Other cases where g_malloc(sizeof(*E)) or g_malloc(sizeof(T)) is used
will either be due to Coccinelle not understanding the types, or due to
a bug in Qemu, and both of these cases need special consideration.

Cheers,
-- 
Stuart