qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/2] Use g_new() & friends where that makes obvi


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 2/2] Use g_new() & friends where that makes obvious sense
Date: Mon, 03 Feb 2014 09:56:17 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux)

Eric Blake <address@hidden> writes:

> On 01/31/2014 08:53 AM, Markus Armbruster wrote:
>> g_new(T, n) is safer than g_malloc(sizeof(T) * n) 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.
>> 
>> Patch created with the following Coccinelle patch, with two hunks
>> dropped from the result:
>> 
>
> Looks like a reasonable formula for replacement.
>
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
>>  186 files changed, 375 insertions(+), 414 deletions(-)
>
> Is there any easy way to enforce that this style does not creep back
> into the code base?  In other words, can you do a followup patch to
> checkpatch.pl that flags use of g_malloc[0]?

There are many, many uses of g_malloc(), g_malloc0() and g_realloc()
left.  The ones with a size argument of the form sizeof(E) * N could be
rewritten, too.  But there are plenty of others.

For the sizeof(E) * N cases, rewriting may not be an improvement.  The
pattern "p = malloc(sizeof(*p))" is pretty idiomatic.

Having checkpatch.pl match the unwanted patterns sizeof(T), sizeof(T)*N,
and their variations seems infeasible to me, because deciding whether T
is a type is beyond its capabilities.

Ideas?

>> @@ -658,7 +658,7 @@ int qcow2_snapshot_list(BlockDriverState *bs, 
>> QEMUSnapshotInfo **psn_tab)
>>          return s->nb_snapshots;
>>      }
>>  
>> -    sn_tab = g_malloc0(s->nb_snapshots * sizeof(QEMUSnapshotInfo));
>> +    sn_tab = g_new0(QEMUSnapshotInfo, s->nb_snapshots);
>>      for(i = 0; i < s->nb_snapshots; i++) {
>
> Should checkpatch.pl care about the spacing after 'for'?  Then again,
> it's unrelated to your conversion, so omitting a whitespace cleanup from
> this patch doesn't hurt.

It already does as far as I can tell, but as Richard said, it only
complains about style violations in changed lines.

>> +++ b/hw/char/virtio-serial-bus.c
>> @@ -921,10 +921,8 @@ static void
>> virtio_serial_device_realize(DeviceState *dev, Error **errp)
>>      QTAILQ_INIT(&vser->ports);
>>  
>>      vser->bus.max_nr_ports = vser->serial.max_virtserial_ports;
>> -    vser->ivqs = g_malloc(vser->serial.max_virtserial_ports
>> -                          * sizeof(VirtQueue *));
>> -    vser->ovqs = g_malloc(vser->serial.max_virtserial_ports
>> -                          * sizeof(VirtQueue *));
>> +    vser->ivqs = g_new(VirtQueue *, vser->serial.max_virtserial_ports);
>> +    vser->ovqs = g_new(VirtQueue *, vser->serial.max_virtserial_ports);
>
> I'm impressed at what Coccinelle can rewrite!

Yup.  It's quite capable, but its language is weird, and its error
messages can compete with C++ is mysteriousness (thankfully not in
length).

>> +++ b/ui/keymaps.c
>> @@ -107,7 +107,7 @@ static kbd_layout_t *parse_keyboard_layout(const
>> name2keysym_t *table,
>>      }
>>  
>>      if (!k)
>> -    k = g_malloc0(sizeof(kbd_layout_t));
>> +        k = g_new0(kbd_layout_t, 1);
>>  
>>      for(;;) {
>
> Fixing tab damage while you are at it - nice.  And just noticed this is
> another instance of no space after 'for', but not worth fixing in this
> patch if checkpatch.pl is happy.
>
> I'm trusting coccinelle, and only glanced through random spots of the
> patch; but where I looked, I didn't find any problems in the conversion.
>
> I don't know if that means we should add:
> Reviewed-by: Eric Blake <address@hidden>

Thank you!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]