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 <address@hidden>
> ---
>   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 <address@hidden>