Patchwork [5/5] Convert remaining calls to g_malloc(sizeof(type)) to g_new()

login
register
mail settings
Submitter Stuart Brady
Date Oct. 20, 2011, 8:03 a.m.
Message ID <1319097820-4788-5-git-send-email-sdb@zubnet.me.uk>
Download mbox | patch
Permalink /patch/120752/
State New
Headers show

Comments

Stuart Brady - Oct. 20, 2011, 8:03 a.m.
Convert calls to g_malloc() and g_malloc0() to g_new() and g_new0()
respectively, in cases where the size passed to g_malloc() is specified
as sizeof(type).

Coccinelle did not match these when matching assignments involving an
expression corresponding to the type used for determining the size to
allocate.  Such cases deserve more careful review, in case the types do
not match, so are submitted separately.

This was achieved using Coccinelle with the following semantic patch:

@@ type T; @@
-g_malloc(sizeof(T))
+g_new(T, 1)

@@ type T; @@
-g_malloc0(sizeof(T))
+g_new0(T, 1)

@@ type T; expression N; @@
-g_malloc(sizeof(T) * N)
+g_new(T, N)

@@ type T; expression N; @@
-g_malloc0(sizeof(T) * N)
+g_new0(T, N)

Note: changes to omap_l4_io_readb_fn, etc in hw/omap_l4.c have been
withheld since the size of the wrong type of pointer is specified.
This should not cause a problem as all types of pointer will have the
same size on any architecture that we care about, but this should be
fixed properly in a separate patch.

Also note that the allocation of seek_data in ga/guest-agent-commands.c
is not fixed because it should use GuestFileSeek, not GuestFileRead.
This should be fixed separately.

Finally, cris-dis.c appears to perform allocations using the wrong
pointer type.  This should also be fixed separately.

Signed-off-by: Stuart Brady <sdb@zubnet.me.uk>
---
 block/qcow2-snapshot.c |    2 +-
 block/vvfat.c          |    2 +-
 bsd-user/syscall.c     |    2 +-
 cpus.c                 |    8 ++++----
 hw/ide/ahci.c          |    2 +-
 hw/intel-hda.c         |    2 +-
 hw/lsi53c895a.c        |    2 +-
 hw/virtio-serial-bus.c |    4 ++--
 hw/virtio.c            |    2 +-
 linux-user/elfload.c   |    2 +-
 monitor.c              |    2 +-
 savevm.c               |    4 ++--
 test-qmp-commands.c    |    2 +-
 ui/vnc.c               |    2 +-
 xen-all.c              |    2 +-
 15 files changed, 20 insertions(+), 20 deletions(-)
Paolo Bonzini - Oct. 20, 2011, 9:05 a.m.
On 10/20/2011 10:03 AM, Stuart Brady wrote:
> Coccinelle did not match these when matching assignments involving an
> expression corresponding to the type used for determining the size to
> allocate.

They all look okay, perhaps the include path you passed to Coccinelle is 
incomplete?

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

but would be nicer if you fixed the problem.

Paolo
Stuart Brady - Oct. 21, 2011, 12:26 a.m.
On Thu, Oct 20, 2011 at 11:05:33AM +0200, Paolo Bonzini wrote:
> On 10/20/2011 10:03 AM, Stuart Brady wrote:
> >Coccinelle did not match these when matching assignments involving an
> >expression corresponding to the type used for determining the size to
> >allocate.
> 
> 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...

> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> but would be nicer if you fixed the problem.

Agreed... I'm new to Coccinelle, but I'll experiment and see if I can
produce a better patch.  After applying my patch series, it seems there
are still some additional calls that need converting, so I can readily
believe that the semantic patches I've used can be improved!

Cheers,
Paolo Bonzini - Oct. 21, 2011, 7:37 a.m.
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".

Paolo
Stuart Brady - Oct. 21, 2011, 5:56 p.m.
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,
Blue Swirl - Oct. 23, 2011, 1:45 p.m.
On Fri, Oct 21, 2011 at 00:26, Stuart Brady <sdb@zubnet.me.uk> wrote:
> On Thu, Oct 20, 2011 at 11:05:33AM +0200, Paolo Bonzini wrote:
>> On 10/20/2011 10:03 AM, Stuart Brady wrote:
>> >Coccinelle did not match these when matching assignments involving an
>> >expression corresponding to the type used for determining the size to
>> >allocate.
>>
>> 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...

Yes, but I can't find the commands from command line history, sorry.
Note to self: always make scripts from complex command invocations and
remember to use the Internet mirrors for storage.

>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> but would be nicer if you fixed the problem.
>
> Agreed... I'm new to Coccinelle, but I'll experiment and see if I can
> produce a better patch.  After applying my patch series, it seems there
> are still some additional calls that need converting, so I can readily
> believe that the semantic patches I've used can be improved!
>
> Cheers,
> --
> Stuart
>
>

Patch

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 4c170d86..1c2470d 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -76,7 +76,7 @@  int qcow2_read_snapshots(BlockDriverState *bs)
     }
 
     offset = s->snapshots_offset;
-    s->snapshots = g_malloc0(s->nb_snapshots * sizeof(QCowSnapshot));
+    s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
     for(i = 0; i < s->nb_snapshots; i++) {
         offset = align_offset(offset, 8);
         if (bdrv_pread(bs->file, offset, &h, sizeof(h)) != sizeof(h))
diff --git a/block/vvfat.c b/block/vvfat.c
index 7e9e35a..1ed9641 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2782,7 +2782,7 @@  static int enable_write_target(BDRVVVFATState *s)
 
     s->bs->backing_hd = calloc(sizeof(BlockDriverState), 1);
     s->bs->backing_hd->drv = &vvfat_write_target;
-    s->bs->backing_hd->opaque = g_malloc(sizeof(void*));
+    s->bs->backing_hd->opaque = g_new(void *, 1);
     *(void**)s->bs->backing_hd->opaque = s;
 
     return 0;
diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 18b43f1..e75ca81 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -231,7 +231,7 @@  static abi_long do_freebsd_sysctl(abi_ulong namep, int32_t namelen, abi_ulong ol
     void *hnamep, *holdp, *hnewp = NULL;
     size_t holdlen;
     abi_ulong oldlen = 0;
-    int32_t *snamep = g_malloc(sizeof(int32_t) * namelen), *p, *q, i;
+    int32_t *snamep = g_new(int32_t, namelen), *p, *q, i;
     uint32_t kind = 0;
 
     if (oldlenp)
diff --git a/cpus.c b/cpus.c
index 8978779..cbe7531 100644
--- a/cpus.c
+++ b/cpus.c
@@ -818,8 +818,8 @@  static void qemu_tcg_init_vcpu(void *_env)
 
     /* share a single thread for all cpus with TCG */
     if (!tcg_cpu_thread) {
-        env->thread = g_malloc0(sizeof(QemuThread));
-        env->halt_cond = g_malloc0(sizeof(QemuCond));
+        env->thread = g_new0(QemuThread, 1);
+        env->halt_cond = g_new0(QemuCond, 1);
         qemu_cond_init(env->halt_cond);
         tcg_halt_cond = env->halt_cond;
         qemu_thread_create(env->thread, qemu_tcg_cpu_thread_fn, env);
@@ -835,8 +835,8 @@  static void qemu_tcg_init_vcpu(void *_env)
 
 static void qemu_kvm_start_vcpu(CPUState *env)
 {
-    env->thread = g_malloc0(sizeof(QemuThread));
-    env->halt_cond = g_malloc0(sizeof(QemuCond));
+    env->thread = g_new0(QemuThread, 1);
+    env->halt_cond = g_new0(QemuCond, 1);
     qemu_cond_init(env->halt_cond);
     qemu_thread_create(env->thread, qemu_kvm_cpu_thread_fn, env);
     while (env->created == 0) {
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1c7e3a0..7f176e3 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1164,7 +1164,7 @@  void ahci_init(AHCIState *s, DeviceState *qdev, int ports)
     int i;
 
     s->ports = ports;
-    s->dev = g_malloc0(sizeof(AHCIDevice) * ports);
+    s->dev = g_new0(AHCIDevice, ports);
     ahci_reg_init(s);
     /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
     memory_region_init_io(&s->mem, &ahci_mem_ops, s, "ahci", AHCI_MEM_BAR_SIZE);
diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 4272204..47a35e4 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -468,7 +468,7 @@  static void intel_hda_parse_bdl(IntelHDAState *d, IntelHDAStream *st)
     addr = intel_hda_addr(st->bdlp_lbase, st->bdlp_ubase);
     st->bentries = st->lvi +1;
     g_free(st->bpl);
-    st->bpl = g_malloc(sizeof(bpl) * st->bentries);
+    st->bpl = g_new(bpl, st->bentries);
     for (i = 0; i < st->bentries; i++, addr += 16) {
         cpu_physical_memory_read(addr, buf, 16);
         st->bpl[i].addr  = le64_to_cpu(*(uint64_t *)buf);
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index e077ec0..110fafd 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -778,7 +778,7 @@  static void lsi_do_command(LSIState *s)
     }
 
     assert(s->current == NULL);
-    s->current = g_malloc0(sizeof(lsi_request));
+    s->current = g_new0(lsi_request, 1);
     s->current->tag = s->select_tag;
     s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, buf,
                                    s->current);
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index a4825b9..9842576 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -862,8 +862,8 @@  VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf)
     QTAILQ_INIT(&vser->ports);
 
     vser->bus.max_nr_ports = conf->max_virtserial_ports;
-    vser->ivqs = g_malloc(conf->max_virtserial_ports * sizeof(VirtQueue *));
-    vser->ovqs = g_malloc(conf->max_virtserial_ports * sizeof(VirtQueue *));
+    vser->ivqs = g_new(VirtQueue *, conf->max_virtserial_ports);
+    vser->ovqs = g_new(VirtQueue *, conf->max_virtserial_ports);
 
     /* Add a queue for host to guest transfers for port 0 (backward compat) */
     vser->ivqs[0] = virtio_add_queue(vdev, 128, handle_input);
diff --git a/hw/virtio.c b/hw/virtio.c
index 7011b5b..c03c50a 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -871,7 +871,7 @@  VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
     vdev->isr = 0;
     vdev->queue_sel = 0;
     vdev->config_vector = VIRTIO_NO_VECTOR;
-    vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
+    vdev->vq = g_new0(VirtQueue, VIRTIO_PCI_QUEUE_MAX);
     vdev->vm_running = runstate_is_running();
     for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
         vdev->vq[i].vector = VIRTIO_NO_VECTOR;
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 56abc8c..fdd83cf 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2488,7 +2488,7 @@  static int fill_note_info(struct elf_note_info *info,
 
     QTAILQ_INIT(&info->thread_list);
 
-    info->notes = g_malloc0(NUMNOTES * sizeof (struct memelfnote));
+    info->notes = g_new0(struct memelfnote, NUMNOTES);
     if (info->notes == NULL)
         return (-ENOMEM);
     info->prstatus = g_malloc0(sizeof (*info->prstatus));
diff --git a/monitor.c b/monitor.c
index 87ce79f..fe45c24 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5124,7 +5124,7 @@  void monitor_init(CharDriverState *chr, int flags)
     }
 
     if (monitor_ctrl_mode(mon)) {
-        mon->mc = g_malloc0(sizeof(MonitorControl));
+        mon->mc = g_new0(MonitorControl, 1);
         /* Control mode requires special handlers */
         qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
                               monitor_control_event, mon);
diff --git a/savevm.c b/savevm.c
index 51f71f7..cce4e7d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1132,7 +1132,7 @@  int register_savevm_live(DeviceState *dev,
             pstrcat(se->idstr, sizeof(se->idstr), "/");
             g_free(id);
 
-            se->compat = g_malloc0(sizeof(CompatEntry));
+            se->compat = g_new0(CompatEntry, 1);
             pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), idstr);
             se->compat->instance_id = instance_id == -1 ?
                          calculate_compat_instance_id(idstr) : instance_id;
@@ -1243,7 +1243,7 @@  int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
             pstrcat(se->idstr, sizeof(se->idstr), "/");
             g_free(id);
 
-            se->compat = g_malloc0(sizeof(CompatEntry));
+            se->compat = g_new0(CompatEntry, 1);
             pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), vmsd->name);
             se->compat->instance_id = instance_id == -1 ?
                          calculate_compat_instance_id(vmsd->name) : instance_id;
diff --git a/test-qmp-commands.c b/test-qmp-commands.c
index f44b6df..d0d8a83 100644
--- a/test-qmp-commands.c
+++ b/test-qmp-commands.c
@@ -120,7 +120,7 @@  static void test_dealloc_types(void)
 
     ud1list = g_new0(UserDefOneList, 1);
     ud1list->value = ud1a;
-    ud1list->next = g_malloc0(sizeof(UserDefOneList));
+    ud1list->next = g_new0(UserDefOneList, 1);
     ud1list->next->value = ud1b;
 
     qapi_free_UserDefOneList(ud1list);
diff --git a/ui/vnc.c b/ui/vnc.c
index 4e28e34..51235dd 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2515,7 +2515,7 @@  static void vnc_connect(VncDisplay *vd, int csock, int skipauth)
 
     vs->lossy_rect = g_malloc0(VNC_STAT_ROWS * sizeof (*vs->lossy_rect));
     for (i = 0; i < VNC_STAT_ROWS; ++i) {
-        vs->lossy_rect[i] = g_malloc0(VNC_STAT_COLS * sizeof (uint8_t));
+        vs->lossy_rect[i] = g_new0(uint8_t, VNC_STAT_COLS);
     }
 
     VNC_DEBUG("New client on socket %d\n", csock);
diff --git a/xen-all.c b/xen-all.c
index 415fde6..ec001aa 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -924,7 +924,7 @@  int xen_hvm_init(void)
         hw_error("map buffered IO page returned error %d", errno);
     }
 
-    state->ioreq_local_port = g_malloc0(smp_cpus * sizeof (evtchn_port_t));
+    state->ioreq_local_port = g_new0(evtchn_port_t, smp_cpus);
 
     /* FIXME: how about if we overflow the page here? */
     for (i = 0; i < smp_cpus; i++) {