Re: [Qemu-devel] [PATCH] arm: Use g_new() & friends where that makes obvious sense

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 08/25/2015 11:39 AM, Markus Armbruster wrote:
>> g_new(T, n) is neater than g_malloc(sizeof(T) * n).  It's also safer,
>> for two reasons.  One, it catches multiplication overflowing size_t.
>> Two, it returns T * rather than void *, which lets the compiler catch
>> more type errors.
>> 
>> This commit only touches allocations with size arguments of the form
>> sizeof(T).
>> 
>> Coccinelle semantic patch:
>> 
>>     @@
>>     type T;
>>     @@
>>     -g_malloc(sizeof(T))
>>     +g_new(T, 1)
>>     @@
>>     type T;
>
> I find it slightly easier to read coccinelle if there is a blank line
> before every second @@, to call attention to metatype vs. changes
> (coccinelle doesn't care either way).
>
> And it's probably possible to write the coccinelle more succinctly, by
> grouping similar rules together using something like this (although I
> didn't actually test it):
>
> @@
> type T;
> @@
> (
>  g_malloc
> |
>  g_try_malloc
> |
>  g_malloc0
> |
>  g_try_malloc0
> )
> -(sizeof(T))
> +(T, 1)
>
> but it doesn't matter in the long run.

I tend to write really stupid semantic patches, because when I try to
write a clever one, I usually relearn I'm not nearly clever enough for
that.

Amazing tool all the same.

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
> Both the coccinelle rule and the resulting conversion look sane.
> Reviewed-by: Eric Blake <address@hidden>
>
>> +++ b/hw/gpio/omap_gpio.c
>> @@ -710,8 +710,8 @@ static int omap2_gpio_init(SysBusDevice *sbd)
>>      } else {
>>          s->modulecount = 6;
>>      }
>> -    s->modules = g_malloc0(s->modulecount * sizeof(struct omap2_gpio_s));
>> -    s->handler = g_malloc0(s->modulecount * 32 * sizeof(qemu_irq));
>> +    s->modules = g_new0(struct omap2_gpio_s, s->modulecount);
>> +    s->handler = g_new0(qemu_irq, s->modulecount * 32);
>>      qdev_init_gpio_in(dev, omap2_gpio_set, s->modulecount * 32);
>>      qdev_init_gpio_out(dev, s->handler, s->modulecount * 32);
>
> Thankfully, the '* 32' here does not overflow even pre-patch, since
> s->modulecount was set to a maximum of 6 in the preceding if/else block.

Thanks!