Re: [Qemu-devel] [PATCH 1/1] Converted malloc calls to g_malloc and g_new

[Mriyam Tamuli]

Thanks a lot for the feedback. I will do as you suggested. I have two more questions, though -

1. So after a patch is submitted, there will be a review in sometime by someone? Is that how I will come to know if my changes are correct?

2. If I split the patch into smaller parts and send to qemu-trivial, do I need to cc qemu-devel?

Warm regards,
Mriyam

On Sat 12 Mar, 2016 10:50 pm Stefan Weil, <address@hidden> wrote:

Am 12.03.2016 um 12:51 schrieb Mriyam Tamuli:
> Signed-off-by: Mriyam Tamuli <address@hidden>
> ---
>  block/iscsi.c      |  2 +-
>  bsd-user/elfload.c | 12 ++++++------
>  bsd-user/qemu.h    |  2 +-
>  configure          |  4 ++--
>  disas/ia64.c       |  2 +-
>  5 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 128ea79..2d6e5b4 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -840,7 +840,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
>          return &acb->common;
>      }
>
> -    acb->task = malloc(sizeof(struct scsi_task));
> +    acb->task = g_malloc(sizeof(struct scsi_task));

I suggest using g_new(struct scsi_task, 1) here.
The following NULL check should be removed as it is not needed with
g_malloc / g_new.

>      if (acb->task == NULL) {
>          error_report("iSCSI: Failed to allocate task for scsi command. %s",
>                       iscsi_get_error(iscsi));
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 0a6092b..74d7c79 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -868,7 +868,7 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
>              return ~(abi_ulong)0UL;
>
>          elf_phdata =  (struct elf_phdr *)
> -                malloc(sizeof(struct elf_phdr) * interp_elf_ex->e_phnum);
> +                g_malloc(sizeof(struct elf_phdr) * interp_elf_ex->e_phnum);

Use g_new and remove type cast and following NULL check.

>
>          if (!elf_phdata)
>            return ~((abi_ulong)0UL);
> @@ -1064,13 +1064,13 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>
>   found:
>      /* Now know where the strtab and symtab are.  Snarf them. */
> -    s = malloc(sizeof(*s));
> -    syms = malloc(symtab.sh_size);
> +    s = g_malloc(sizeof(*s));
Maybe g_new here.
> +    syms = g_malloc(symtab.sh_size);

No NULL check needed.

>      if (!syms) {
>          free(s);
>          return;
>      }
> -    s->disas_strtab = strings = malloc(strtab.sh_size);
> +    s->disas_strtab = strings = g_malloc(strtab.sh_size);

No NULL check needed.

>      if (!s->disas_strtab) {
>          free(s);
>          free(syms);
> @@ -1191,7 +1191,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>      }
>
>      /* Now read in all of the header information */
> -    elf_phdata = (struct elf_phdr *)malloc(elf_ex.e_phentsize*elf_ex.e_phnum);
> +    elf_phdata = g_new(elf_ex.e_phentsize * elf_ex.e_phnum);

g_new(elf_ex.e_phentsize, elf_ex.e_phnum)

No NULL check needed.

>      if (elf_phdata == NULL) {
>          return -ENOMEM;
>      }
> @@ -1244,7 +1244,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>               * is an a.out format binary
>               */
>
> -            elf_interpreter = (char *)malloc(elf_ppnt->p_filesz);
> +            elf_interpreter = g_new(elf_ppnt->p_filesz);

Wrong number of parameters.

No NULL check needed.

>
>              if (elf_interpreter == NULL) {
>                  free (elf_phdata);
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index 03b502a..ada4360 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -357,7 +357,7 @@ static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy
>  #ifdef DEBUG_REMAP
>      {
>          void *addr;
> -        addr = malloc(len);
> +        addr = g_malloc(len);
>          if (copy)
>              memcpy(addr, g2h(guest_addr), len);
>          else
> diff --git a/configure b/configure
> index 2b32876..5df672b 100755
> --- a/configure
> +++ b/configure
> @@ -3512,7 +3512,7 @@ fi
>  if test "$tcmalloc" = "yes" ; then
>    cat > $TMPC << EOF
>  #include <stdlib.h>
> -int main(void) { malloc(1); return 0; }
> +int main(void) { g_malloc(1); return 0; }

I don't think that replacing malloc is correct here,
because that test wants to test for libtcmalloc.

>  EOF
>
>    if compile_prog "" "-ltcmalloc" ; then
> @@ -3528,7 +3528,7 @@ fi
>  if test "$jemalloc" = "yes" ; then
>    cat > $TMPC << EOF
>  #include <stdlib.h>
> -int main(void) { malloc(1); return 0; }
> +int main(void) { g_malloc(1); return 0; }

Similar case here, don't replace malloc for this test.

>  EOF
>
>    if compile_prog "" "-ljemalloc" ; then
> diff --git a/disas/ia64.c b/disas/ia64.c
> index 140754c..b0733ed 100644
> --- a/disas/ia64.c
> +++ b/disas/ia64.c
> @@ -10268,7 +10268,7 @@ static struct ia64_opcode *
>  make_ia64_opcode (ia64_insn opcode, const char *name, int place, int depind)
>  {
>    struct ia64_opcode *res =
> -    (struct ia64_opcode *) malloc (sizeof (struct ia64_opcode));
> +    g_new(sizeof(struct ia64_opcode));

Wrong number (and value) of parameters for g_new.

>    res->name = strdup (name);
>    res->type = main_table[place].opcode_type;
>    res->num_outputs = main_table[place].num_outputs;

In general, for each replaced malloc there should
normally be (at least) one replaced free (missing
in your patch). Replacing malloc is a good thing in
most cases (see file HACKING, section 3).

I suggest to split your patch in smaller parts which only
replace malloc / free for one file and remove any type casts
or NULL checks which then are no longer needed.

Those smaller parts can be sent fo qemu-trivial and
will be accepted there after a successful review.

Kind regards,
Stefan W.