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

["Alex Bennée" <alex.bennee@linaro.org>]

Mriyam Tamuli <mbtamuli@gmail.com> writes:

> 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?

To answer the first part first. You are responsible for testing your
changes. For simple things like this a compile check is usually enough
(although make check is good practice).

For more involved changes you should either ensure the code is exercised
by the unit test code or manually devise a test that exercises the code
paths you have just changed. For some more involved things this can be
quite hard to do - it is not unknown to add printfs as a temporary patch
to check something gets exercised while booting a kernel and then
removing the printfs for the submission ;-)

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

Generally yes. qemu-devel is the master development mailing list. Other
lists like qemu-trivial and qemu-$ARCH are convenience lists that help
with sorting patches out for reviewers.

>
> Warm regards,
> Mriyam
>
> On Sat 12 Mar, 2016 10:50 pm Stefan Weil, <sw@weilnetz.de> wrote:
>
>> Am 12.03.2016 um 12:51 schrieb Mriyam Tamuli:
>> > Signed-off-by: Mriyam Tamuli <mbtamuli@gmail.com>
>> > ---
>> >  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.
>>
>>


--
Alex Bennée