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, <s...@weilnetz.de> wrote:
> Am 12.03.2016 um 12:51 schrieb Mriyam Tamuli:
> > Signed-off-by: Mriyam Tamuli <mbtam...@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.
>
>
