| Message ID | 1457783505-21956-1-git-send-email-mbtamuli@gmail.com |
|---|---|
| State | New |
| Headers | show |
Can anyone please comment on this? Am I doing anything wrong here? On Sat, Mar 12, 2016 at 5:22 PM Mriyam Tamuli <mbtamuli@gmail.com> wrote: > > 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)); > 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); > > 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)); > + syms = g_malloc(symtab.sh_size); > if (!syms) { > free(s); > return; > } > - s->disas_strtab = strings = malloc(strtab.sh_size); > + s->disas_strtab = strings = g_malloc(strtab.sh_size); > 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); > 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); > > 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; } > 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; } > 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)); > res->name = strdup (name); > res->type = main_table[place].opcode_type; > res->num_outputs = main_table[place].num_outputs; > -- > 2.5.0 >
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.
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, <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. > >
Am 12.03.2016 um 18:53 schrieb 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? Yes, that's how it should work. But it can also happen that a patch does not get any attention. In that case you can send a polite reminder. See http://wiki.qemu.org/Contribute/SubmitAPatch for more important information on submitting patches. > > 2. If I split the patch into smaller parts and send to qemu-trivial, do > I need to cc qemu-devel? Yes, trivial patches should be sent to qemu-trivial, qemu-devel and any maintainers of the modified code. Stefan
Mriyam Tamuli <mbtamuli@gmail.com> writes: > Signed-off-by: Mriyam Tamuli <mbtamuli@gmail.com> While this is a good change you still need to ensure any corresponding free's are converted to g_free (and g_delete for g_new). > --- > 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)); > if (acb->task == NULL) { g_malloc can't fail so the error leg can be removed. > 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); > > if (!elf_phdata) > return ~((abi_ulong)0UL); Again error checking becomes redundant. > @@ -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)); > + syms = g_malloc(symtab.sh_size); > if (!syms) { > free(s); > return; > } And here. > - s->disas_strtab = strings = malloc(strtab.sh_size); > + s->disas_strtab = strings = g_malloc(strtab.sh_size); > if (!s->disas_strtab) { > free(s); > free(syms); And here > @@ -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); > 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); > > if (elf_interpreter == NULL) { And here. > 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 wouldn't touch the configure cases. There are used for probing the library support and in this case I don't see how it woould work considering only stdlib.h was included. > 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; } > 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)); > res->name = strdup (name); > res->type = main_table[place].opcode_type; > res->num_outputs = main_table[place].num_outputs; -- Alex Bennée
Mriyam Tamuli <mbtamuli@gmail.com> writes: > Can anyone please comment on this? Am I doing anything wrong here? It's best to wait a few days. Most QEMU hackers work 9-5 jobs so activity on the mailing list is fairly light over the weekend. > > On Sat, Mar 12, 2016 at 5:22 PM Mriyam Tamuli <mbtamuli@gmail.com> wrote: >> >> 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)); >> 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); >> >> 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)); >> + syms = g_malloc(symtab.sh_size); >> if (!syms) { >> free(s); >> return; >> } >> - s->disas_strtab = strings = malloc(strtab.sh_size); >> + s->disas_strtab = strings = g_malloc(strtab.sh_size); >> 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); >> 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); >> >> 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; } >> 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; } >> 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)); >> res->name = strdup (name); >> res->type = main_table[place].opcode_type; >> res->num_outputs = main_table[place].num_outputs; >> -- >> 2.5.0 >> -- Alex Bennée
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
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)); 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); 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)); + syms = g_malloc(symtab.sh_size); if (!syms) { free(s); return; } - s->disas_strtab = strings = malloc(strtab.sh_size); + s->disas_strtab = strings = g_malloc(strtab.sh_size); 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); 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); 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; } 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; } 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)); res->name = strdup (name); res->type = main_table[place].opcode_type; res->num_outputs = main_table[place].num_outputs;
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(-)