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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

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 <armbru@redhat.com>
> ---

>  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]?


> @@ -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.

> +++ 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!

> +++ 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 <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]