diff mbox

block: Simplify a few g_try_malloc() error checks

Message ID 1422361514-25226-1-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Jan. 27, 2015, 12:25 p.m. UTC
Unlike malloc(), g_try_malloc() & friends return a null pointer only
on failure, never for a zero size.  Simplify tests for failure
accordingly.  This helps Coverity see returned null pointers can't be
dereferenced.  Also makes the code easier to read.

Tracked down with this Coccinelle semantic patch:

    @@
    type T, TT;
    identifier LHS;
    expression SZ, SZ2, N, E;
    @@
    (
            T *LHS = g_try_malloc(SZ);
    |
            T *LHS = g_try_malloc0(SZ);
    |
            T *LHS = g_try_new(TT, N);
    |
            T *LHS = g_try_new0(TT, N);
    |
            T *LHS = g_try_realloc(E, SZ);
    )
            ...
    -       if (SZ2 && LHS == NULL)
    +       if (!LHS)
            {
                ...
            }
    @@
    type TT;
    expression LHS, SZ, SZ2, N, E;
    @@
    (
            LHS = g_try_malloc(SZ);
    |
            LHS = g_try_malloc0(SZ);
    |
            LHS = g_try_new(TT, N);
    |
            LHS = g_try_new0(TT, N)
    |
            LHS = g_try_realloc(E, SZ);
    )
            ...
    -       if (SZ2 && LHS == NULL)
    +       if (!LHS)
            {
                ...
            }

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/bochs.c          | 2 +-
 block/curl.c           | 2 +-
 block/nfs.c            | 2 +-
 block/parallels.c      | 2 +-
 block/qcow2-refcount.c | 6 +++---
 block/qcow2-snapshot.c | 4 ++--
 block/qed-check.c      | 2 +-
 block/vdi.c            | 2 +-
 block/vhdx.c           | 2 +-
 block/vmdk.c           | 4 ++--
 10 files changed, 14 insertions(+), 14 deletions(-)

Comments

Paolo Bonzini Jan. 27, 2015, 1:01 p.m. UTC | #1
On 27/01/2015 13:25, Markus Armbruster wrote:
> Unlike malloc(), g_try_malloc() & friends return a null pointer only
> on failure, never for a zero size.  Simplify tests for failure
> accordingly.  This helps Coverity see returned null pointers can't be
> dereferenced.  Also makes the code easier to read.

Unfortunately that's not what I see from the source:

gpointer
g_try_malloc (gsize n_bytes)
{
  gpointer mem;

  if (G_LIKELY (n_bytes))
    mem = glib_mem_vtable.try_malloc (n_bytes);
  else
    mem = NULL;

  TRACE (GLIB_MEM_ALLOC((void*) mem, (unsigned int) n_bytes, 0, 1));

  return mem;
}

Paolo
Markus Armbruster Jan. 27, 2015, 1:42 p.m. UTC | #2
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 27/01/2015 13:25, Markus Armbruster wrote:
>> Unlike malloc(), g_try_malloc() & friends return a null pointer only
>> on failure, never for a zero size.  Simplify tests for failure
>> accordingly.  This helps Coverity see returned null pointers can't be
>> dereferenced.  Also makes the code easier to read.
>
> Unfortunately that's not what I see from the source:
>
> gpointer
> g_try_malloc (gsize n_bytes)
> {
>   gpointer mem;
>
>   if (G_LIKELY (n_bytes))
>     mem = glib_mem_vtable.try_malloc (n_bytes);
>   else
>     mem = NULL;
>
>   TRACE (GLIB_MEM_ALLOC((void*) mem, (unsigned int) n_bytes, 0, 1));
>
>   return mem;
> }

You're right.  Brain fart, please ignore.
Kevin Wolf Jan. 27, 2015, 2:07 p.m. UTC | #3
Am 27.01.2015 um 14:42 hat Markus Armbruster geschrieben:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 27/01/2015 13:25, Markus Armbruster wrote:
> >> Unlike malloc(), g_try_malloc() & friends return a null pointer only
> >> on failure, never for a zero size.  Simplify tests for failure
> >> accordingly.  This helps Coverity see returned null pointers can't be
> >> dereferenced.  Also makes the code easier to read.
> >
> > Unfortunately that's not what I see from the source:
> >
> > gpointer
> > g_try_malloc (gsize n_bytes)
> > {
> >   gpointer mem;
> >
> >   if (G_LIKELY (n_bytes))
> >     mem = glib_mem_vtable.try_malloc (n_bytes);
> >   else
> >     mem = NULL;
> >
> >   TRACE (GLIB_MEM_ALLOC((void*) mem, (unsigned int) n_bytes, 0, 1));
> >
> >   return mem;
> > }
> 
> You're right.  Brain fart, please ignore.

Should we consider introducing a qemu_try_malloc() that has the desired
behaviour? The (g_try_)malloc() interface is really easy to misuse and
I've messed up some error checks in the block layer myself.

Kevin
diff mbox

Patch

diff --git a/block/bochs.c b/block/bochs.c
index 199ac2b..c52d2af 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -132,7 +132,7 @@  static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->catalog_bitmap = g_try_new(uint32_t, s->catalog_size);
-    if (s->catalog_size && s->catalog_bitmap == NULL) {
+    if (!s->catalog_bitmap) {
         error_setg(errp, "Could not allocate memory for catalog");
         return -ENOMEM;
     }
diff --git a/block/curl.c b/block/curl.c
index bbee3ca..6709dfc 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -666,7 +666,7 @@  static void curl_readv_bh_cb(void *p)
     state->buf_len = acb->end + s->readahead_size;
     end = MIN(start + state->buf_len, s->len) - 1;
     state->orig_buf = g_try_malloc(state->buf_len);
-    if (state->buf_len && state->orig_buf == NULL) {
+    if (!state->orig_buf) {
         curl_clean_state(state);
         acb->common.cb(acb->common.opaque, -ENOMEM);
         qemu_aio_unref(acb);
diff --git a/block/nfs.c b/block/nfs.c
index ca9e24e..eb5517d 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -173,7 +173,7 @@  static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
     nfs_co_init_task(client, &task);
 
     buf = g_try_malloc(nb_sectors * BDRV_SECTOR_SIZE);
-    if (nb_sectors && buf == NULL) {
+    if (!buf) {
         return -ENOMEM;
     }
 
diff --git a/block/parallels.c b/block/parallels.c
index 4f9cd8d..9fa64d8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -122,7 +122,7 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
     s->catalog_bitmap = g_try_new(uint32_t, s->catalog_size);
-    if (s->catalog_size && s->catalog_bitmap == NULL) {
+    if (!s->catalog_bitmap) {
         ret = -ENOMEM;
         goto fail;
     }
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9afdb40..6d8b833 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -893,7 +893,7 @@  int qcow2_update_snapshot_refcount(BlockDriverState *bs,
      * when changing this! */
     if (l1_table_offset != s->l1_table_offset) {
         l1_table = g_try_malloc0(align_offset(l1_size2, 512));
-        if (l1_size2 && l1_table == NULL) {
+        if (!l1_table) {
             ret = -ENOMEM;
             goto fail;
         }
@@ -1572,7 +1572,7 @@  static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
 
     if (!*refcount_table) {
         *refcount_table = g_try_new0(uint16_t, *nb_clusters);
-        if (*nb_clusters && *refcount_table == NULL) {
+        if (!*refcount_table) {
             res->check_errors++;
             return -ENOMEM;
         }
@@ -2183,7 +2183,7 @@  int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
             uint64_t *l1 = g_try_malloc(l1_sz2);
             int ret;
 
-            if (l1_sz2 && l1 == NULL) {
+            if (!l1) {
                 return -ENOMEM;
             }
 
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5b3903c..7eb5c7d 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -382,7 +382,7 @@  int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     sn->l1_size = s->l1_size;
 
     l1_table = g_try_new(uint64_t, s->l1_size);
-    if (s->l1_size && l1_table == NULL) {
+    if (!l1_table) {
         ret = -ENOMEM;
         goto fail;
     }
@@ -505,7 +505,7 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
      * table is overwritten.
      */
     sn_l1_table = g_try_malloc0(cur_l1_bytes);
-    if (cur_l1_bytes && sn_l1_table == NULL) {
+    if (!sn_l1_table) {
         ret = -ENOMEM;
         goto fail;
     }
diff --git a/block/qed-check.c b/block/qed-check.c
index 36ecd29..3b6b2a4 100644
--- a/block/qed-check.c
+++ b/block/qed-check.c
@@ -228,7 +228,7 @@  int qed_check(BDRVQEDState *s, BdrvCheckResult *result, bool fix)
     int ret;
 
     check.used_clusters = g_try_new0(uint32_t, (check.nclusters + 31) / 32);
-    if (check.nclusters && check.used_clusters == NULL) {
+    if (!check.used_clusters) {
         return -ENOMEM;
     }
 
diff --git a/block/vdi.c b/block/vdi.c
index 74030c6..1a97a27 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -305,7 +305,7 @@  static int vdi_check(BlockDriverState *bs, BdrvCheckResult *res,
     }
 
     bmap = g_try_new(uint32_t, s->header.blocks_in_image);
-    if (s->header.blocks_in_image && bmap == NULL) {
+    if (!bmap) {
         res->check_errors++;
         return -ENOMEM;
     }
diff --git a/block/vhdx.c b/block/vhdx.c
index 06f2b1a..67fcebf 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1597,7 +1597,7 @@  static int vhdx_create_bat(BlockDriverState *bs, BDRVVHDXState *s,
                 bdrv_has_zero_init(bs) == 0) {
         /* for a fixed file, the default BAT entry is not zero */
         s->bat = g_try_malloc0(length);
-        if (length && s->bat == NULL) {
+        if (!s->bat) {
             ret = -ENOMEM;
             goto exit;
         }
diff --git a/block/vmdk.c b/block/vmdk.c
index 52cb888..20360ef 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -456,7 +456,7 @@  static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
     /* read the L1 table */
     l1_size = extent->l1_size * sizeof(uint32_t);
     extent->l1_table = g_try_malloc(l1_size);
-    if (l1_size && extent->l1_table == NULL) {
+    if (!extent->l1_table) {
         return -ENOMEM;
     }
 
@@ -476,7 +476,7 @@  static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
 
     if (extent->l1_backup_table_offset) {
         extent->l1_backup_table = g_try_malloc(l1_size);
-        if (l1_size && extent->l1_backup_table == NULL) {
+        if (!extent->l1_backup_table) {
             ret = -ENOMEM;
             goto fail_l1;
         }