[Qemu-devel] [PATCH v2 0/4] block: Use g_new() & friends more

[Qemu-devel] [PATCH v2 0/4] block: Use g_new() & friends more

[Markus Armbruster]

PATCH 1+2 convert some allocations.  While preparing them, I stumbled
over dead error handling and some useless casts, which led to PATCH
3+4.

I posted a tree-wide version of PATCH 1 some time ago, and was told to
split it up.  This is the block part, redone from scratch.  Other
parts available on request.

v2:
* Regenerated with spatch.
* Because of that, I decided not to carry Max's R-by forward.

Markus Armbruster (4):
  block: Use g_new() & friends where that makes obvious sense
  block: Use g_new() & friends to avoid multiplying sizes
  qemu-io-cmds: g_renew() can't fail, bury dead error handling
  block: Drop some superfluous casts from void *

 block-migration.c      |  6 +++---
 block.c                | 14 +++++++-------
 block/archipelago.c    |  6 +++---
 block/bochs.c          |  2 +-
 block/gluster.c        |  8 ++++----
 block/iscsi.c          |  2 +-
 block/nfs.c            |  2 +-
 block/parallels.c      |  2 +-
 block/qcow.c           |  2 +-
 block/qcow2-cache.c    |  2 +-
 block/qcow2-cluster.c  |  2 +-
 block/qcow2-refcount.c |  8 ++++----
 block/qcow2-snapshot.c |  8 ++++----
 block/qed-check.c      |  3 +--
 block/raw-posix.c      |  2 +-
 block/rbd.c            |  6 +++---
 block/sheepdog.c       |  6 +++---
 block/vdi.c            |  2 +-
 block/vhdx-log.c       |  2 +-
 block/vhdx.c           |  4 ++--
 block/vmdk.c           |  7 +++----
 block/vvfat.c          | 10 +++++-----
 blockdev-nbd.c         |  2 +-
 blockdev.c             |  2 +-
 hw/block/nvme.c        |  8 ++++----
 hw/ide/ahci.c          |  2 +-
 hw/ide/microdrive.c    |  2 +-
 qemu-io-cmds.c         | 21 ++++++---------------
 qemu-io.c              |  2 +-
 29 files changed, 67 insertions(+), 78 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/4] block: Use g_new() & friends where that makes obvious sense

[Markus Armbruster]

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.

Patch created with Coccinelle, with two manual changes on top:

* Add const to bdrv_iterate_format() to keep the types straight

* Convert the allocation in bdrv_drop_intermediate(), which Coccinelle
  inexplicably misses

Coccinelle semantic patch:

    @@
    type T;
    @@
    -g_malloc(sizeof(T))
    +g_new(T, 1)
    @@
    type T;
    @@
    -g_try_malloc(sizeof(T))
    +g_try_new(T, 1)
    @@
    type T;
    @@
    -g_malloc0(sizeof(T))
    +g_new0(T, 1)
    @@
    type T;
    @@
    -g_try_malloc0(sizeof(T))
    +g_try_new0(T, 1)
    @@
    type T;
    expression n;
    @@
    -g_malloc(sizeof(T) * (n))
    +g_new(T, n)
    @@
    type T;
    expression n;
    @@
    -g_try_malloc(sizeof(T) * (n))
    +g_try_new(T, n)
    @@
    type T;
    expression n;
    @@
    -g_malloc0(sizeof(T) * (n))
    +g_new0(T, n)
    @@
    type T;
    expression n;
    @@
    -g_try_malloc0(sizeof(T) * (n))
    +g_try_new0(T, n)
    @@
    type T;
    expression p, n;
    @@
    -g_realloc(p, sizeof(T) * (n))
    +g_renew(T, p, n)
    @@
    type T;
    expression p, n;
    @@
    -g_try_realloc(p, sizeof(T) * (n))
    +g_try_renew(T, p, n)

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block-migration.c      |  6 +++---
 block.c                | 14 +++++++-------
 block/archipelago.c    |  6 +++---
 block/gluster.c        |  8 ++++----
 block/iscsi.c          |  2 +-
 block/nfs.c            |  2 +-
 block/qcow.c           |  2 +-
 block/qcow2-cluster.c  |  2 +-
 block/qcow2-refcount.c |  8 ++++----
 block/qcow2-snapshot.c |  8 ++++----
 block/raw-posix.c      |  2 +-
 block/rbd.c            |  4 ++--
 block/sheepdog.c       |  4 ++--
 block/vdi.c            |  2 +-
 block/vhdx.c           |  4 ++--
 block/vmdk.c           |  7 +++----
 block/vvfat.c          |  2 +-
 blockdev-nbd.c         |  2 +-
 blockdev.c             |  2 +-
 hw/ide/ahci.c          |  2 +-
 qemu-io-cmds.c         |  2 +-
 qemu-io.c              |  2 +-
 22 files changed, 46 insertions(+), 47 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index ba3ed36..3ad31a2 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -283,7 +283,7 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
         nr_sectors = total_sectors - cur_sector;
     }
 
-    blk = g_malloc(sizeof(BlkMigBlock));
+    blk = g_new(BlkMigBlock, 1);
     blk->buf = g_malloc(BLOCK_SIZE);
     blk->bmds = bmds;
     blk->sector = cur_sector;
@@ -354,7 +354,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
             return;
         }
 
-        bmds = g_malloc0(sizeof(BlkMigDevState));
+        bmds = g_new0(BlkMigDevState, 1);
         bmds->bs = bs;
         bmds->bulk_completed = 0;
         bmds->total_sectors = sectors;
@@ -465,7 +465,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
             } else {
                 nr_sectors = BDRV_SECTORS_PER_DIRTY_CHUNK;
             }
-            blk = g_malloc(sizeof(BlkMigBlock));
+            blk = g_new(BlkMigBlock, 1);
             blk->buf = g_malloc(BLOCK_SIZE);
             blk->bmds = bmds;
             blk->sector = sector;
diff --git a/block.c b/block.c
index 6fa0201..712f5db 100644
--- a/block.c
+++ b/block.c
@@ -351,7 +351,7 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp)
         return NULL;
     }
 
-    bs = g_malloc0(sizeof(BlockDriverState));
+    bs = g_new0(BlockDriverState, 1);
     QLIST_INIT(&bs->dirty_bitmaps);
     pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
     if (device_name[0] != '\0') {
@@ -2628,7 +2628,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
      * into our deletion queue, until we hit the 'base'
      */
     while (intermediate) {
-        intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
+        intermediate_state = g_new0(BlkIntermediateStates, 1);
         intermediate_state->bs = intermediate;
         QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
 
@@ -3755,7 +3755,7 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
             }
 
             if (!found) {
-                formats = g_realloc(formats, (count + 1) * sizeof(char *));
+                formats = g_renew(const char *, formats, count + 1);
                 formats[count++] = drv->format_name;
                 it(opaque, drv->format_name);
             }
@@ -5330,7 +5330,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
         errno = -bitmap_size;
         return NULL;
     }
-    bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
+    bitmap = g_new0(BdrvDirtyBitmap, 1);
     bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     return bitmap;
@@ -5356,8 +5356,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
     BlockDirtyInfoList **plist = &list;
 
     QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
-        BlockDirtyInfo *info = g_malloc0(sizeof(BlockDirtyInfo));
-        BlockDirtyInfoList *entry = g_malloc0(sizeof(BlockDirtyInfoList));
+        BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
+        BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
         info->count = bdrv_get_dirty_count(bs, bm);
         info->granularity =
             ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
@@ -5451,7 +5451,7 @@ void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason)
     BdrvOpBlocker *blocker;
     assert((int) op >= 0 && op < BLOCK_OP_TYPE_MAX);
 
-    blocker = g_malloc0(sizeof(BdrvOpBlocker));
+    blocker = g_new0(BdrvOpBlocker, 1);
     blocker->reason = reason;
     QLIST_INSERT_HEAD(&bs->op_blockers[op], blocker, list);
 }
diff --git a/block/archipelago.c b/block/archipelago.c
index 6629d03..34f72dc 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -750,7 +750,7 @@ static int archipelago_submit_request(BDRVArchipelagoState *s,
     char *target;
     void *data = NULL;
     struct xseg_request *req;
-    AIORequestData *reqdata = g_malloc(sizeof(AIORequestData));
+    AIORequestData *reqdata = g_new(AIORequestData, 1);
 
     targetlen = strlen(s->volname);
     req = xseg_get_request(s->xseg, s->srcport, s->vportno, X_ALLOC);
@@ -827,7 +827,7 @@ static int archipelago_aio_segmented_rw(BDRVArchipelagoState *s,
     int i, ret, segments_nr, last_segment_size;
     ArchipelagoSegmentedRequest *segreq;
 
-    segreq = g_malloc(sizeof(ArchipelagoSegmentedRequest));
+    segreq = g_new(ArchipelagoSegmentedRequest, 1);
 
     if (op == ARCHIP_OP_FLUSH) {
         segments_nr = 1;
@@ -960,7 +960,7 @@ static int64_t archipelago_volume_info(BDRVArchipelagoState *s)
     int ret, targetlen;
     struct xseg_request *req;
     struct xseg_reply_info *xinfo;
-    AIORequestData *reqdata = g_malloc(sizeof(AIORequestData));
+    AIORequestData *reqdata = g_new(AIORequestData, 1);
 
     const char *volname = s->volname;
     targetlen = strlen(volname);
diff --git a/block/gluster.c b/block/gluster.c
index 9274dea..1912cf9 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -291,7 +291,7 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     BDRVGlusterState *s = bs->opaque;
     int open_flags = 0;
     int ret = 0;
-    GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
+    GlusterConf *gconf = g_new0(GlusterConf, 1);
     QemuOpts *opts;
     Error *local_err = NULL;
     const char *filename;
@@ -351,12 +351,12 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
     assert(state != NULL);
     assert(state->bs != NULL);
 
-    state->opaque = g_malloc0(sizeof(BDRVGlusterReopenState));
+    state->opaque = g_new0(BDRVGlusterReopenState, 1);
     reop_s = state->opaque;
 
     qemu_gluster_parse_flags(state->flags, &open_flags);
 
-    gconf = g_malloc0(sizeof(GlusterConf));
+    gconf = g_new0(GlusterConf, 1);
 
     reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
     if (reop_s->glfs == NULL) {
@@ -486,7 +486,7 @@ static int qemu_gluster_create(const char *filename,
     int prealloc = 0;
     int64_t total_size = 0;
     char *tmp = NULL;
-    GlusterConf *gconf = g_malloc0(sizeof(GlusterConf));
+    GlusterConf *gconf = g_new0(GlusterConf, 1);
 
     glfs = qemu_gluster_init(gconf, filename, errp);
     if (!glfs) {
diff --git a/block/iscsi.c b/block/iscsi.c
index 2c9cfc1..cdd19c2 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1532,7 +1532,7 @@ static int iscsi_create(const char *filename, QemuOpts *opts, Error **errp)
     /* Read out options */
     total_size =
         qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE;
-    bs->opaque = g_malloc0(sizeof(struct IscsiLun));
+    bs->opaque = g_new0(struct IscsiLun, 1);
     iscsilun = bs->opaque;
 
     bs_options = qdict_new();
diff --git a/block/nfs.c b/block/nfs.c
index fe46c33..93d87f3 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -409,7 +409,7 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp)
 {
     int ret = 0;
     int64_t total_size = 0;
-    NFSClient *client = g_malloc0(sizeof(NFSClient));
+    NFSClient *client = g_new0(NFSClient, 1);
 
     client->aio_context = qemu_get_aio_context();
 
diff --git a/block/qcow.c b/block/qcow.c
index 67332f0..67c237f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -182,7 +182,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->l1_table_offset = header.l1_table_offset;
-    s->l1_table = g_try_malloc(s->l1_size * sizeof(uint64_t));
+    s->l1_table = g_try_new(uint64_t, s->l1_size);
     if (s->l1_table == NULL) {
         error_setg(errp, "Could not allocate memory for L1 table");
         ret = -ENOMEM;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 5b36018..735f687 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -711,7 +711,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
     trace_qcow2_cluster_link_l2(qemu_coroutine_self(), m->nb_clusters);
     assert(m->nb_clusters > 0);
 
-    old_cluster = g_try_malloc(m->nb_clusters * sizeof(uint64_t));
+    old_cluster = g_try_new(uint64_t, m->nb_clusters);
     if (old_cluster == NULL) {
         ret = -ENOMEM;
         goto err;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3b77470..43665b8 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -350,7 +350,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
     uint64_t meta_offset = (blocks_used * refcount_block_clusters) *
         s->cluster_size;
     uint64_t table_offset = meta_offset + blocks_clusters * s->cluster_size;
-    uint64_t *new_table = g_try_malloc0(table_size * sizeof(uint64_t));
+    uint64_t *new_table = g_try_new0(uint64_t, table_size);
     uint16_t *new_blocks = g_try_malloc0(blocks_clusters * s->cluster_size);
 
     assert(table_size > 0 && blocks_clusters > 0);
@@ -1524,7 +1524,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
         return -EFBIG;
     }
 
-    refcount_table = g_try_malloc0(nb_clusters * sizeof(uint16_t));
+    refcount_table = g_try_new0(uint16_t, nb_clusters);
     if (nb_clusters && refcount_table == NULL) {
         res->check_errors++;
         return -ENOMEM;
@@ -1605,8 +1605,8 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                         /* increase refcount_table size if necessary */
                         int old_nb_clusters = nb_clusters;
                         nb_clusters = (new_offset >> s->cluster_bits) + 1;
-                        refcount_table = g_realloc(refcount_table,
-                                nb_clusters * sizeof(uint16_t));
+                        refcount_table = g_renew(uint16_t, refcount_table,
+                                                 nb_clusters);
                         memset(&refcount_table[old_nb_clusters], 0, (nb_clusters
                                 - old_nb_clusters) * sizeof(uint16_t));
                     }
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index f67b472..f52d7fd 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -58,7 +58,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++) {
         /* Read statically sized part of the snapshot header */
@@ -381,7 +381,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     sn->l1_table_offset = l1_table_offset;
     sn->l1_size = s->l1_size;
 
-    l1_table = g_try_malloc(s->l1_size * sizeof(uint64_t));
+    l1_table = g_try_new(uint64_t, s->l1_size);
     if (s->l1_size && l1_table == NULL) {
         ret = -ENOMEM;
         goto fail;
@@ -417,7 +417,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     }
 
     /* Append the new snapshot to the snapshot list */
-    new_snapshot_list = g_malloc((s->nb_snapshots + 1) * sizeof(QCowSnapshot));
+    new_snapshot_list = g_new(QCowSnapshot, s->nb_snapshots + 1);
     if (s->snapshots) {
         memcpy(new_snapshot_list, s->snapshots,
                s->nb_snapshots * sizeof(QCowSnapshot));
@@ -661,7 +661,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++) {
         sn_info = sn_tab + i;
         sn = s->snapshots + i;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 1194eb0..5c745b9 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -517,7 +517,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
 
     s = state->bs->opaque;
 
-    state->opaque = g_malloc0(sizeof(BDRVRawReopenState));
+    state->opaque = g_new0(BDRVRawReopenState, 1);
     raw_s = state->opaque;
 
 #ifdef CONFIG_LINUX_AIO
diff --git a/block/rbd.c b/block/rbd.c
index 4459102..3aaf855 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -652,7 +652,7 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
     off = sector_num * BDRV_SECTOR_SIZE;
     size = nb_sectors * BDRV_SECTOR_SIZE;
 
-    rcb = g_malloc(sizeof(RADOSCB));
+    rcb = g_new(RADOSCB, 1);
     rcb->done = 0;
     rcb->acb = acb;
     rcb->buf = buf;
@@ -873,7 +873,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
         goto done;
     }
 
-    sn_tab = g_malloc0(snap_count * sizeof(QEMUSnapshotInfo));
+    sn_tab = g_new0(QEMUSnapshotInfo, snap_count);
 
     for (i = 0; i < snap_count; i++) {
         const char *snap_name = snaps[i].name;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 8d9350c..ba1ef43 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1682,7 +1682,7 @@ static int sd_create(const char *filename, QemuOpts *opts,
     uint32_t snapid;
     bool prealloc = false;
 
-    s = g_malloc0(sizeof(BDRVSheepdogState));
+    s = g_new0(BDRVSheepdogState, 1);
 
     memset(tag, 0, sizeof(tag));
     if (strstr(filename, "://")) {
@@ -2273,7 +2273,7 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     uint32_t snapid = 0;
     int ret = 0;
 
-    old_s = g_malloc(sizeof(BDRVSheepdogState));
+    old_s = g_new(BDRVSheepdogState, 1);
 
     memcpy(old_s, s, sizeof(BDRVSheepdogState));
 
diff --git a/block/vdi.c b/block/vdi.c
index adc6aa9..4b10aac 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -292,7 +292,7 @@ static int vdi_check(BlockDriverState *bs, BdrvCheckResult *res,
         return -ENOTSUP;
     }
 
-    bmap = g_try_malloc(s->header.blocks_in_image * sizeof(uint32_t));
+    bmap = g_try_new(uint32_t, s->header.blocks_in_image);
     if (s->header.blocks_in_image && bmap == NULL) {
         res->check_errors++;
         return -ENOMEM;
diff --git a/block/vhdx.c b/block/vhdx.c
index f666940..87c99fc 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1381,7 +1381,7 @@ static int vhdx_create_new_headers(BlockDriverState *bs, uint64_t image_size,
     int ret = 0;
     VHDXHeader *hdr = NULL;
 
-    hdr = g_malloc0(sizeof(VHDXHeader));
+    hdr = g_new0(VHDXHeader, 1);
 
     hdr->signature       = VHDX_HEADER_SIGNATURE;
     hdr->sequence_number = g_random_int();
@@ -1654,7 +1654,7 @@ static int vhdx_create_new_region_table(BlockDriverState *bs,
 
     /* Populate enough of the BDRVVHDXState to be able to use the
      * pre-existing BAT calculation, translation, and update functions */
-    s = g_malloc0(sizeof(BDRVVHDXState));
+    s = g_new0(BDRVVHDXState, 1);
 
     s->chunk_ratio = (VHDX_MAX_SECTORS_PER_BLOCK) *
                      (uint64_t) sector_size / (uint64_t) block_size;
diff --git a/block/vmdk.c b/block/vmdk.c
index 01412a8..f000d2a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -233,7 +233,7 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
         return;
     }
     s->num_extents--;
-    s->extents = g_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
+    s->extents = g_renew(VmdkExtent, s->extents, s->num_extents);
 }
 
 static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
@@ -418,8 +418,7 @@ static int vmdk_add_extent(BlockDriverState *bs,
         return length;
     }
 
-    s->extents = g_realloc(s->extents,
-                              (s->num_extents + 1) * sizeof(VmdkExtent));
+    s->extents = g_renew(VmdkExtent, s->extents, s->num_extents + 1);
     extent = &s->extents[s->num_extents];
     s->num_extents++;
 
@@ -497,7 +496,7 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
     }
 
     extent->l2_cache =
-        g_malloc(extent->l2_size * L2_CACHE_SIZE * sizeof(uint32_t));
+        g_new(uint32_t, extent->l2_size * L2_CACHE_SIZE);
     return 0;
  fail_l1b:
     g_free(extent->l1_backup_table);
diff --git a/block/vvfat.c b/block/vvfat.c
index 70176b1..f877e85 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2950,7 +2950,7 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp)
 
     bdrv_set_backing_hd(s->bs, bdrv_new("", &error_abort));
     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/blockdev-nbd.c b/blockdev-nbd.c
index b3a2474..06f901e 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -108,7 +108,7 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
 
     nbd_export_set_name(exp, device);
 
-    n = g_malloc0(sizeof(NBDCloseNotifier));
+    n = g_new0(NBDCloseNotifier, 1);
     n->n.notify = nbd_close_notifier;
     n->exp = exp;
     bdrv_add_close_notifier(bs, &n->n);
diff --git a/blockdev.c b/blockdev.c
index 48bd9a3..8acd264 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1094,7 +1094,7 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
         return NULL;
     }
 
-    info = g_malloc0(sizeof(SnapshotInfo));
+    info = g_new0(SnapshotInfo, 1);
     info->id = g_strdup(sn.id_str);
     info->name = g_strdup(sn.name);
     info->date_nsec = sn.date_nsec;
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4cda0d0..5a10bac 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1203,7 +1203,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
 
     s->as = as;
     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, OBJECT(qdev), &ahci_mem_ops, s,
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index c503fc6..3a1e11e 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -114,7 +114,7 @@ static char **breakline(char *input, int *count)
 {
     int c = 0;
     char *p;
-    char **rval = g_malloc0(sizeof(char *));
+    char **rval = g_new0(char *, 1);
     char **tmp;
 
     while (rval && (p = qemu_strsep(&input, " ")) != NULL) {
diff --git a/qemu-io.c b/qemu-io.c
index b55a550..33c96c4 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -356,7 +356,7 @@ static void command_loop(void)
 
 static void add_user_command(char *optarg)
 {
-    cmdline = g_realloc(cmdline, ++ncmdline * sizeof(char *));
+    cmdline = g_renew(char *, cmdline, ++ncmdline);
     cmdline[ncmdline-1] = optarg;
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/4] block: Use g_new() & friends to avoid multiplying sizes

[Markus Armbruster]

g_new(T, n) is safer than g_malloc(sizeof(*v) * 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.

Perhaps a conversion to g_malloc_n() would be neater in places, but
that's merely four years old, and we can't use such newfangled stuff.

This commit only touches allocations with size arguments of the form
sizeof(T), plus two that use 4 instead of sizeof(uint32_t).  We can
make the others safe by converting to g_malloc_n() when it becomes
available to us in a couple of years.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/bochs.c       |  2 +-
 block/parallels.c   |  2 +-
 block/qcow2-cache.c |  2 +-
 block/qed-check.c   |  3 +--
 block/rbd.c         |  2 +-
 block/sheepdog.c    |  2 +-
 hw/block/nvme.c     |  8 ++++----
 qemu-io-cmds.c      | 10 +++++-----
 8 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/block/bochs.c b/block/bochs.c
index 6674b27..199ac2b 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -131,7 +131,7 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
         return -EFBIG;
     }
 
-    s->catalog_bitmap = g_try_malloc(s->catalog_size * 4);
+    s->catalog_bitmap = g_try_new(uint32_t, s->catalog_size);
     if (s->catalog_size && s->catalog_bitmap == NULL) {
         error_setg(errp, "Could not allocate memory for catalog");
         return -ENOMEM;
diff --git a/block/parallels.c b/block/parallels.c
index 1774ab8..2a814f3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -121,7 +121,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EFBIG;
         goto fail;
     }
-    s->catalog_bitmap = g_try_malloc(s->catalog_size * 4);
+    s->catalog_bitmap = g_try_new(uint32_t, s->catalog_size);
     if (s->catalog_size && s->catalog_bitmap == NULL) {
         ret = -ENOMEM;
         goto fail;
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 5353b44..fe0615a 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -50,7 +50,7 @@ Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables)
 
     c = g_malloc0(sizeof(*c));
     c->size = num_tables;
-    c->entries = g_malloc0(sizeof(*c->entries) * num_tables);
+    c->entries = g_new0(Qcow2CachedTable, num_tables);
 
     for (i = 0; i < c->size; i++) {
         c->entries[i].table = qemu_try_blockalign(bs->file, s->cluster_size);
diff --git a/block/qed-check.c b/block/qed-check.c
index 40a882c..36ecd29 100644
--- a/block/qed-check.c
+++ b/block/qed-check.c
@@ -227,8 +227,7 @@ int qed_check(BDRVQEDState *s, BdrvCheckResult *result, bool fix)
     };
     int ret;
 
-    check.used_clusters = g_try_malloc0(((check.nclusters + 31) / 32) *
-                                        sizeof(check.used_clusters[0]));
+    check.used_clusters = g_try_new0(uint32_t, (check.nclusters + 31) / 32);
     if (check.nclusters && check.used_clusters == NULL) {
         return -ENOMEM;
     }
diff --git a/block/rbd.c b/block/rbd.c
index 3aaf855..ea969e7 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -862,7 +862,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
     int max_snaps = RBD_MAX_SNAPS;
 
     do {
-        snaps = g_malloc(sizeof(*snaps) * max_snaps);
+        snaps = g_new(rbd_snap_info_t, max_snaps);
         snap_count = rbd_snap_list(s->image, snaps, &max_snaps);
         if (snap_count <= 0) {
             g_free(snaps);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index ba1ef43..12cbd9d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2357,7 +2357,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
         goto out;
     }
 
-    sn_tab = g_malloc0(nr * sizeof(*sn_tab));
+    sn_tab = g_new0(QEMUSnapshotInfo, nr);
 
     /* calculate a vdi id with hash function */
     hval = fnv_64a_buf(s->name, strlen(s->name), FNV1A_64_INIT);
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5fd8f89..47577ec 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -319,7 +319,7 @@ static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
     sq->size = size;
     sq->cqid = cqid;
     sq->head = sq->tail = 0;
-    sq->io_req = g_malloc(sq->size * sizeof(*sq->io_req));
+    sq->io_req = g_new(NvmeRequest, sq->size);
 
     QTAILQ_INIT(&sq->req_list);
     QTAILQ_INIT(&sq->out_req_list);
@@ -773,9 +773,9 @@ static int nvme_init(PCIDevice *pci_dev)
     n->reg_size = 1 << qemu_fls(0x1004 + 2 * (n->num_queues + 1) * 4);
     n->ns_size = bs_size / (uint64_t)n->num_namespaces;
 
-    n->namespaces = g_malloc0(sizeof(*n->namespaces)*n->num_namespaces);
-    n->sq = g_malloc0(sizeof(*n->sq)*n->num_queues);
-    n->cq = g_malloc0(sizeof(*n->cq)*n->num_queues);
+    n->namespaces = g_new0(NvmeNamespace, n->num_namespaces);
+    n->sq = g_new0(NvmeSQueue *, n->num_queues);
+    n->cq = g_new0(NvmeCQueue *, n->num_queues);
 
     memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n,
                           "nvme", n->reg_size);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 3a1e11e..afd8867 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -29,7 +29,7 @@ static int compare_cmdname(const void *a, const void *b)
 
 void qemuio_add_command(const cmdinfo_t *ci)
 {
-    cmdtab = g_realloc(cmdtab, ++ncmds * sizeof(*cmdtab));
+    cmdtab = g_renew(cmdinfo_t, cmdtab, ++ncmds);
     cmdtab[ncmds - 1] = *ci;
     qsort(cmdtab, ncmds, sizeof(*cmdtab), compare_cmdname);
 }
@@ -122,7 +122,7 @@ static char **breakline(char *input, int *count)
             continue;
         }
         c++;
-        tmp = g_realloc(rval, sizeof(*rval) * (c + 1));
+        tmp = g_renew(char *, rval, (c + 1));
         if (!tmp) {
             g_free(rval);
             rval = NULL;
@@ -1264,9 +1264,9 @@ static int multiwrite_f(BlockDriverState *bs, int argc, char **argv)
         }
     }
 
-    reqs = g_malloc0(nr_reqs * sizeof(*reqs));
-    buf = g_malloc0(nr_reqs * sizeof(*buf));
-    qiovs = g_malloc(nr_reqs * sizeof(*qiovs));
+    reqs = g_new0(BlockRequest, nr_reqs);
+    buf = g_new0(char *, nr_reqs);
+    qiovs = g_new(QEMUIOVector, nr_reqs);
 
     for (i = 0; i < nr_reqs && optind < argc; i++) {
         int j;
-- 
1.9.3

[Qemu-devel] [PATCH v2 3/4] qemu-io-cmds: g_renew() can't fail, bury dead error handling

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-io-cmds.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index afd8867..b224ede 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -115,22 +115,13 @@ static char **breakline(char *input, int *count)
     int c = 0;
     char *p;
     char **rval = g_new0(char *, 1);
-    char **tmp;
 
     while (rval && (p = qemu_strsep(&input, " ")) != NULL) {
         if (!*p) {
             continue;
         }
         c++;
-        tmp = g_renew(char *, rval, (c + 1));
-        if (!tmp) {
-            g_free(rval);
-            rval = NULL;
-            c = 0;
-            break;
-        } else {
-            rval = tmp;
-        }
+        rval = g_renew(char *, rval, (c + 1));
         rval[c - 1] = p;
         rval[c] = NULL;
     }
-- 
1.9.3

[Qemu-devel] [PATCH v2 4/4] block: Drop some superfluous casts from void *

[Markus Armbruster]

They clutter the code.  Unfortunately, I can't figure out how to make
Coccinelle drop all of them, so I have to settle for common special
cases:

    @@
    type T;
    T *pt;
    void *pv;
    @@
    - pt = (T *)pv;
    + pt = pv;
    @@
    type T;
    @@
    - (T *)
      (\(g_malloc\|g_malloc0\|g_realloc\|g_new\|g_new0\|g_renew\|
	 g_try_malloc\|g_try_malloc0\|g_try_realloc\|
	 g_try_new\|g_try_new0\|g_try_renew\)(...))

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vhdx-log.c    | 2 +-
 block/vvfat.c       | 8 ++++----
 hw/ide/microdrive.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index eb5c7a0..4267e60 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -923,7 +923,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
     buffer = qemu_blockalign(bs, total_length);
     memcpy(buffer, &new_hdr, sizeof(new_hdr));
 
-    new_desc = (VHDXLogDescriptor *) (buffer + sizeof(new_hdr));
+    new_desc = (buffer + sizeof(new_hdr));
     data_sector = buffer + (desc_sectors * VHDX_LOG_SECTOR_SIZE);
     data_tmp = data;
 
diff --git a/block/vvfat.c b/block/vvfat.c
index f877e85..401539d 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -732,7 +732,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
 	if(first_cluster == 0 && (is_dotdot || is_dot))
 	    continue;
 
-	buffer=(char*)g_malloc(length);
+	buffer=g_malloc(length);
 	snprintf(buffer,length,"%s/%s",dirname,entry->d_name);
 
 	if(stat(buffer,&st)<0) {
@@ -767,7 +767,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
 
 	/* create mapping for this file */
 	if(!is_dot && !is_dotdot && (S_ISDIR(st.st_mode) || st.st_size)) {
-	    s->current_mapping=(mapping_t*)array_get_next(&(s->mapping));
+	    s->current_mapping = array_get_next(&(s->mapping));
 	    s->current_mapping->begin=0;
 	    s->current_mapping->end=st.st_size;
 	    /*
@@ -811,12 +811,12 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
     }
 
      /* reget the mapping, since s->mapping was possibly realloc()ed */
-    mapping = (mapping_t*)array_get(&(s->mapping), mapping_index);
+    mapping = array_get(&(s->mapping), mapping_index);
     first_cluster += (s->directory.next - mapping->info.dir.first_dir_index)
 	* 0x20 / s->cluster_size;
     mapping->end = first_cluster;
 
-    direntry = (direntry_t*)array_get(&(s->directory), mapping->dir_index);
+    direntry = array_get(&(s->directory), mapping->dir_index);
     set_begin_of_direntry(direntry, mapping->begin);
 
     return 0;
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index 2d70ddb..15671b8 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -567,7 +567,7 @@ PCMCIACardState *dscm1xxxx_init(DriveInfo *dinfo)
     }
     md->bus.ifs[0].drive_kind = IDE_CFATA;
     md->bus.ifs[0].mdata_size = METADATA_SIZE;
-    md->bus.ifs[0].mdata_storage = (uint8_t *) g_malloc0(METADATA_SIZE);
+    md->bus.ifs[0].mdata_storage = g_malloc0(METADATA_SIZE);
 
     return PCMCIA_CARD(md);
 }
-- 
1.9.3

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

[Max Reitz]

On 18.08.2014 18:10, 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.
>
> Patch created with Coccinelle, with two manual changes on top:
>
> * Add const to bdrv_iterate_format() to keep the types straight
>
> * Convert the allocation in bdrv_drop_intermediate(), which Coccinelle
>    inexplicably misses
>
> Coccinelle semantic patch:
>
>      @@
>      type T;
>      @@
>      -g_malloc(sizeof(T))
>      +g_new(T, 1)
>      @@
>      type T;
>      @@
>      -g_try_malloc(sizeof(T))
>      +g_try_new(T, 1)
>      @@
>      type T;
>      @@
>      -g_malloc0(sizeof(T))
>      +g_new0(T, 1)
>      @@
>      type T;
>      @@
>      -g_try_malloc0(sizeof(T))
>      +g_try_new0(T, 1)
>      @@
>      type T;
>      expression n;
>      @@
>      -g_malloc(sizeof(T) * (n))
>      +g_new(T, n)
>      @@
>      type T;
>      expression n;
>      @@
>      -g_try_malloc(sizeof(T) * (n))
>      +g_try_new(T, n)
>      @@
>      type T;
>      expression n;
>      @@
>      -g_malloc0(sizeof(T) * (n))
>      +g_new0(T, n)
>      @@
>      type T;
>      expression n;
>      @@
>      -g_try_malloc0(sizeof(T) * (n))
>      +g_try_new0(T, n)
>      @@
>      type T;
>      expression p, n;
>      @@
>      -g_realloc(p, sizeof(T) * (n))
>      +g_renew(T, p, n)
>      @@
>      type T;
>      expression p, n;
>      @@
>      -g_try_realloc(p, sizeof(T) * (n))
>      +g_try_renew(T, p, n)
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block-migration.c      |  6 +++---
>   block.c                | 14 +++++++-------
>   block/archipelago.c    |  6 +++---
>   block/gluster.c        |  8 ++++----
>   block/iscsi.c          |  2 +-
>   block/nfs.c            |  2 +-
>   block/qcow.c           |  2 +-
>   block/qcow2-cluster.c  |  2 +-
>   block/qcow2-refcount.c |  8 ++++----
>   block/qcow2-snapshot.c |  8 ++++----
>   block/raw-posix.c      |  2 +-
>   block/rbd.c            |  4 ++--
>   block/sheepdog.c       |  4 ++--
>   block/vdi.c            |  2 +-
>   block/vhdx.c           |  4 ++--
>   block/vmdk.c           |  7 +++----
>   block/vvfat.c          |  2 +-
>   blockdev-nbd.c         |  2 +-
>   blockdev.c             |  2 +-
>   hw/ide/ahci.c          |  2 +-
>   qemu-io-cmds.c         |  2 +-
>   qemu-io.c              |  2 +-
>   22 files changed, 46 insertions(+), 47 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/4] block: Use g_new() & friends to avoid multiplying sizes

[Max Reitz]

On 18.08.2014 18:10, Markus Armbruster wrote:
> g_new(T, n) is safer than g_malloc(sizeof(*v) * 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.
>
> Perhaps a conversion to g_malloc_n() would be neater in places, but
> that's merely four years old, and we can't use such newfangled stuff.
>
> This commit only touches allocations with size arguments of the form
> sizeof(T), plus two that use 4 instead of sizeof(uint32_t).  We can
> make the others safe by converting to g_malloc_n() when it becomes
> available to us in a couple of years.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/bochs.c       |  2 +-
>   block/parallels.c   |  2 +-
>   block/qcow2-cache.c |  2 +-
>   block/qed-check.c   |  3 +--
>   block/rbd.c         |  2 +-
>   block/sheepdog.c    |  2 +-
>   hw/block/nvme.c     |  8 ++++----
>   qemu-io-cmds.c      | 10 +++++-----
>   8 files changed, 15 insertions(+), 16 deletions(-)

[snip]

> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 3a1e11e..afd8867 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -29,7 +29,7 @@ static int compare_cmdname(const void *a, const void *b)
>   
>   void qemuio_add_command(const cmdinfo_t *ci)
>   {
> -    cmdtab = g_realloc(cmdtab, ++ncmds * sizeof(*cmdtab));
> +    cmdtab = g_renew(cmdinfo_t, cmdtab, ++ncmds);
>       cmdtab[ncmds - 1] = *ci;
>       qsort(cmdtab, ncmds, sizeof(*cmdtab), compare_cmdname);
>   }
> @@ -122,7 +122,7 @@ static char **breakline(char *input, int *count)
>               continue;
>           }
>           c++;
> -        tmp = g_realloc(rval, sizeof(*rval) * (c + 1));
> +        tmp = g_renew(char *, rval, (c + 1));
>           if (!tmp) {
>               g_free(rval);
>               rval = NULL;

Not really relevant for this patch, but: g_renew() never returns NULL if 
n_structs (c + 1) is non-zero, does it? So this if block here should be 
unnecessary, I think.

Anyway:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/4] qemu-io-cmds: g_renew() can't fail, bury dead error handling

[Max Reitz]

On 18.08.2014 18:10, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qemu-io-cmds.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)

Oops, disregard my comment for patch 2. ;-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/4] block: Drop some superfluous casts from void *

[Max Reitz]

On 18.08.2014 18:10, Markus Armbruster wrote:
> They clutter the code.  Unfortunately, I can't figure out how to make
> Coccinelle drop all of them, so I have to settle for common special
> cases:
>
>      @@
>      type T;
>      T *pt;
>      void *pv;
>      @@
>      - pt = (T *)pv;
>      + pt = pv;
>      @@
>      type T;
>      @@
>      - (T *)
>        (\(g_malloc\|g_malloc0\|g_realloc\|g_new\|g_new0\|g_renew\|
> 	 g_try_malloc\|g_try_malloc0\|g_try_realloc\|
> 	 g_try_new\|g_try_new0\|g_try_renew\)(...))
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/vhdx-log.c    | 2 +-
>   block/vvfat.c       | 8 ++++----
>   hw/ide/microdrive.c | 2 +-
>   3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index eb5c7a0..4267e60 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -923,7 +923,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
>       buffer = qemu_blockalign(bs, total_length);
>       memcpy(buffer, &new_hdr, sizeof(new_hdr));
>   
> -    new_desc = (VHDXLogDescriptor *) (buffer + sizeof(new_hdr));
> +    new_desc = (buffer + sizeof(new_hdr));
>       data_sector = buffer + (desc_sectors * VHDX_LOG_SECTOR_SIZE);
>       data_tmp = data;

You could drop the parantheses here. Also, I still don't like void 
pointer arithmetic, but well... ;-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

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

[Jeff Cody]

On Mon, Aug 18, 2014 at 06:10:40PM +0200, 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.
> 
> Patch created with Coccinelle, with two manual changes on top:
> 
> * Add const to bdrv_iterate_format() to keep the types straight
> 
> * Convert the allocation in bdrv_drop_intermediate(), which Coccinelle
>   inexplicably misses
> 
> Coccinelle semantic patch:
> 
>     @@
>     type T;
>     @@
>     -g_malloc(sizeof(T))
>     +g_new(T, 1)
>     @@
>     type T;
>     @@
>     -g_try_malloc(sizeof(T))
>     +g_try_new(T, 1)
>     @@
>     type T;
>     @@
>     -g_malloc0(sizeof(T))
>     +g_new0(T, 1)
>     @@
>     type T;
>     @@
>     -g_try_malloc0(sizeof(T))
>     +g_try_new0(T, 1)
>     @@
>     type T;
>     expression n;
>     @@
>     -g_malloc(sizeof(T) * (n))
>     +g_new(T, n)
>     @@
>     type T;
>     expression n;
>     @@
>     -g_try_malloc(sizeof(T) * (n))
>     +g_try_new(T, n)
>     @@
>     type T;
>     expression n;
>     @@
>     -g_malloc0(sizeof(T) * (n))
>     +g_new0(T, n)
>     @@
>     type T;
>     expression n;
>     @@
>     -g_try_malloc0(sizeof(T) * (n))
>     +g_try_new0(T, n)
>     @@
>     type T;
>     expression p, n;
>     @@
>     -g_realloc(p, sizeof(T) * (n))
>     +g_renew(T, p, n)
>     @@
>     type T;
>     expression p, n;
>     @@
>     -g_try_realloc(p, sizeof(T) * (n))
>     +g_try_renew(T, p, n)
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block-migration.c      |  6 +++---
>  block.c                | 14 +++++++-------
>  block/archipelago.c    |  6 +++---
>  block/gluster.c        |  8 ++++----
>  block/iscsi.c          |  2 +-
>  block/nfs.c            |  2 +-
>  block/qcow.c           |  2 +-
>  block/qcow2-cluster.c  |  2 +-
>  block/qcow2-refcount.c |  8 ++++----
>  block/qcow2-snapshot.c |  8 ++++----
>  block/raw-posix.c      |  2 +-
>  block/rbd.c            |  4 ++--
>  block/sheepdog.c       |  4 ++--
>  block/vdi.c            |  2 +-
>  block/vhdx.c           |  4 ++--
>  block/vmdk.c           |  7 +++----
>  block/vvfat.c          |  2 +-
>  blockdev-nbd.c         |  2 +-
>  blockdev.c             |  2 +-
>  hw/ide/ahci.c          |  2 +-
>  qemu-io-cmds.c         |  2 +-
>  qemu-io.c              |  2 +-
>  22 files changed, 46 insertions(+), 47 deletions(-)

[...]

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/4] block: Use g_new() & friends to avoid multiplying sizes

[Jeff Cody]

On Mon, Aug 18, 2014 at 06:10:41PM +0200, Markus Armbruster wrote:
> g_new(T, n) is safer than g_malloc(sizeof(*v) * 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.
> 
> Perhaps a conversion to g_malloc_n() would be neater in places, but
> that's merely four years old, and we can't use such newfangled stuff.
> 
> This commit only touches allocations with size arguments of the form
> sizeof(T), plus two that use 4 instead of sizeof(uint32_t).  We can
> make the others safe by converting to g_malloc_n() when it becomes
> available to us in a couple of years.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/bochs.c       |  2 +-
>  block/parallels.c   |  2 +-
>  block/qcow2-cache.c |  2 +-
>  block/qed-check.c   |  3 +--
>  block/rbd.c         |  2 +-
>  block/sheepdog.c    |  2 +-
>  hw/block/nvme.c     |  8 ++++----
>  qemu-io-cmds.c      | 10 +++++-----
>  8 files changed, 15 insertions(+), 16 deletions(-)
> 

[...]

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/4] qemu-io-cmds: g_renew() can't fail, bury dead error handling

[Jeff Cody]

On Mon, Aug 18, 2014 at 06:10:42PM +0200, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-io-cmds.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index afd8867..b224ede 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -115,22 +115,13 @@ static char **breakline(char *input, int *count)
>      int c = 0;
>      char *p;
>      char **rval = g_new0(char *, 1);
> -    char **tmp;
>  
>      while (rval && (p = qemu_strsep(&input, " ")) != NULL) {
>          if (!*p) {
>              continue;
>          }
>          c++;
> -        tmp = g_renew(char *, rval, (c + 1));
> -        if (!tmp) {
> -            g_free(rval);
> -            rval = NULL;
> -            c = 0;
> -            break;
> -        } else {
> -            rval = tmp;
> -        }
> +        rval = g_renew(char *, rval, (c + 1));
>          rval[c - 1] = p;
>          rval[c] = NULL;
>      }
> -- 
> 1.9.3
> 
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/4] block: Drop some superfluous casts from void *

[Jeff Cody]

On Mon, Aug 18, 2014 at 06:10:43PM +0200, Markus Armbruster wrote:
> They clutter the code.  Unfortunately, I can't figure out how to make
> Coccinelle drop all of them, so I have to settle for common special
> cases:
> 
>     @@
>     type T;
>     T *pt;
>     void *pv;
>     @@
>     - pt = (T *)pv;
>     + pt = pv;
>     @@
>     type T;
>     @@
>     - (T *)
>       (\(g_malloc\|g_malloc0\|g_realloc\|g_new\|g_new0\|g_renew\|
> 	 g_try_malloc\|g_try_malloc0\|g_try_realloc\|
> 	 g_try_new\|g_try_new0\|g_try_renew\)(...))
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/vhdx-log.c    | 2 +-
>  block/vvfat.c       | 8 ++++----
>  hw/ide/microdrive.c | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
> index eb5c7a0..4267e60 100644
> --- a/block/vhdx-log.c
> +++ b/block/vhdx-log.c
> @@ -923,7 +923,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
>      buffer = qemu_blockalign(bs, total_length);
>      memcpy(buffer, &new_hdr, sizeof(new_hdr));
>  
> -    new_desc = (VHDXLogDescriptor *) (buffer + sizeof(new_hdr));
> +    new_desc = (buffer + sizeof(new_hdr));

Agree with Max, in that the parenthesis could be removed.  Not worthy
of a respin normally, but since the point of this patch is to
unclutter the code, I guess it makes sense to fix it.

>      data_sector = buffer + (desc_sectors * VHDX_LOG_SECTOR_SIZE);
>      data_tmp = data;
>  
> diff --git a/block/vvfat.c b/block/vvfat.c
> index f877e85..401539d 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -732,7 +732,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
>  	if(first_cluster == 0 && (is_dotdot || is_dot))
>  	    continue;
>  
> -	buffer=(char*)g_malloc(length);
> +	buffer=g_malloc(length);

You missed a spot to put spaces around the '=' here.  Nothing worthy
of a respin, of course - just a note in case you decide to respin.


[...]

With or without the changes above:

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/4] block: Drop some superfluous casts from void *

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Mon, Aug 18, 2014 at 06:10:43PM +0200, Markus Armbruster wrote:
>> They clutter the code.  Unfortunately, I can't figure out how to make
>> Coccinelle drop all of them, so I have to settle for common special
>> cases:
>> 
>>     @@
>>     type T;
>>     T *pt;
>>     void *pv;
>>     @@
>>     - pt = (T *)pv;
>>     + pt = pv;
>>     @@
>>     type T;
>>     @@
>>     - (T *)
>>       (\(g_malloc\|g_malloc0\|g_realloc\|g_new\|g_new0\|g_renew\|
>> 	 g_try_malloc\|g_try_malloc0\|g_try_realloc\|
>> 	 g_try_new\|g_try_new0\|g_try_renew\)(...))
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/vhdx-log.c    | 2 +-
>>  block/vvfat.c       | 8 ++++----
>>  hw/ide/microdrive.c | 2 +-
>>  3 files changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
>> index eb5c7a0..4267e60 100644
>> --- a/block/vhdx-log.c
>> +++ b/block/vhdx-log.c
>> @@ -923,7 +923,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
>>      buffer = qemu_blockalign(bs, total_length);
>>      memcpy(buffer, &new_hdr, sizeof(new_hdr));
>>  
>> -    new_desc = (VHDXLogDescriptor *) (buffer + sizeof(new_hdr));
>> +    new_desc = (buffer + sizeof(new_hdr));
>
> Agree with Max, in that the parenthesis could be removed.  Not worthy
> of a respin normally, but since the point of this patch is to
> unclutter the code, I guess it makes sense to fix it.

Max and you are right.  I neglected to clean up the patch produced by
spatch.

>>      data_sector = buffer + (desc_sectors * VHDX_LOG_SECTOR_SIZE);
>>      data_tmp = data;
>>  
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index f877e85..401539d 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -732,7 +732,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
>>  	if(first_cluster == 0 && (is_dotdot || is_dot))
>>  	    continue;
>>  
>> -	buffer=(char*)g_malloc(length);
>> +	buffer=g_malloc(length);
>
> You missed a spot to put spaces around the '=' here.  Nothing worthy
> of a respin, of course - just a note in case you decide to respin.

Right again.  Although perhaps vvfat should remain ugly, to warn unwary
travellers ;)

> [...]
>
> With or without the changes above:
>
> Reviewed-by: Jeff Cody <jcody@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH v2 4/4] block: Drop some superfluous casts from void *

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 18.08.2014 18:10, Markus Armbruster wrote:
>> They clutter the code.  Unfortunately, I can't figure out how to make
>> Coccinelle drop all of them, so I have to settle for common special
>> cases:
>>
>>      @@
>>      type T;
>>      T *pt;
>>      void *pv;
>>      @@
>>      - pt = (T *)pv;
>>      + pt = pv;
>>      @@
>>      type T;
>>      @@
>>      - (T *)
>>        (\(g_malloc\|g_malloc0\|g_realloc\|g_new\|g_new0\|g_renew\|
>> 	 g_try_malloc\|g_try_malloc0\|g_try_realloc\|
>> 	 g_try_new\|g_try_new0\|g_try_renew\)(...))
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   block/vhdx-log.c    | 2 +-
>>   block/vvfat.c       | 8 ++++----
>>   hw/ide/microdrive.c | 2 +-
>>   3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
>> index eb5c7a0..4267e60 100644
>> --- a/block/vhdx-log.c
>> +++ b/block/vhdx-log.c
>> @@ -923,7 +923,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
>>       buffer = qemu_blockalign(bs, total_length);
>>       memcpy(buffer, &new_hdr, sizeof(new_hdr));
>>   -    new_desc = (VHDXLogDescriptor *) (buffer + sizeof(new_hdr));
>> +    new_desc = (buffer + sizeof(new_hdr));
>>       data_sector = buffer + (desc_sectors * VHDX_LOG_SECTOR_SIZE);
>>       data_tmp = data;
>
> You could drop the parantheses here. Also, I still don't like void
> pointer arithmetic, but well... ;-)

Separate issue, related because it would put the cast right back.

We use plenty of GCCisms.  If we ever want to port to a toolchain that
doesn't support them, adding the cast clutter to avoid arithmetic on
void * will be the least of our worries.

If we decide to hunt down this particular GCCism anyway, there's
-Wpointer-arith.  Without that, it's a game of whack-a-mole.

> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH v2 4/4] block: Drop some superfluous casts from void *

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Jeff Cody <jcody@redhat.com> writes:
>
>> On Mon, Aug 18, 2014 at 06:10:43PM +0200, Markus Armbruster wrote:
>>> They clutter the code.  Unfortunately, I can't figure out how to make
>>> Coccinelle drop all of them, so I have to settle for common special
>>> cases:
>>> 
>>>     @@
>>>     type T;
>>>     T *pt;
>>>     void *pv;
>>>     @@
>>>     - pt = (T *)pv;
>>>     + pt = pv;
>>>     @@
>>>     type T;
>>>     @@
>>>     - (T *)
>>>       (\(g_malloc\|g_malloc0\|g_realloc\|g_new\|g_new0\|g_renew\|
>>> 	 g_try_malloc\|g_try_malloc0\|g_try_realloc\|
>>> 	 g_try_new\|g_try_new0\|g_try_renew\)(...))
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  block/vhdx-log.c    | 2 +-
>>>  block/vvfat.c       | 8 ++++----
>>>  hw/ide/microdrive.c | 2 +-
>>>  3 files changed, 6 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/block/vhdx-log.c b/block/vhdx-log.c
>>> index eb5c7a0..4267e60 100644
>>> --- a/block/vhdx-log.c
>>> +++ b/block/vhdx-log.c
>>> @@ -923,7 +923,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s,
>>>      buffer = qemu_blockalign(bs, total_length);
>>>      memcpy(buffer, &new_hdr, sizeof(new_hdr));
>>>  
>>> -    new_desc = (VHDXLogDescriptor *) (buffer + sizeof(new_hdr));
>>> +    new_desc = (buffer + sizeof(new_hdr));
>>
>> Agree with Max, in that the parenthesis could be removed.  Not worthy
>> of a respin normally, but since the point of this patch is to
>> unclutter the code, I guess it makes sense to fix it.
>
> Max and you are right.  I neglected to clean up the patch produced by
> spatch.

Cleaned up in v3.

>>>      data_sector = buffer + (desc_sectors * VHDX_LOG_SECTOR_SIZE);
>>>      data_tmp = data;
>>>  
>>> diff --git a/block/vvfat.c b/block/vvfat.c
>>> index f877e85..401539d 100644
>>> --- a/block/vvfat.c
>>> +++ b/block/vvfat.c
>>> @@ -732,7 +732,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
>>>  	if(first_cluster == 0 && (is_dotdot || is_dot))
>>>  	    continue;
>>>  
>>> -	buffer=(char*)g_malloc(length);
>>> +	buffer=g_malloc(length);
>>
>> You missed a spot to put spaces around the '=' here.  Nothing worthy
>> of a respin, of course - just a note in case you decide to respin.
>
> Right again.  Although perhaps vvfat should remain ugly, to warn unwary
> travellers ;)

Cleaned up in v3.

[...]