| Message ID | 1319097820-4788-5-git-send-email-sdb@zubnet.me.uk |
|---|---|
| State | New |
| Headers | show |
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
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,
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
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,
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 > >
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++) {
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(-)