| Message ID | 169635378817.28428.8916197505999208589-1@git.sr.ht |
|---|---|
| State | New |
| Headers | show |
| Series | contrib/elf2dmp: Conversion of conversions of malloc/calloc/free to g_malloc/g_new/g_free | expand |
On Tue, 3 Oct 2023 at 18:23, ~h0lyalg0rithm <h0lyalg0rithm@git.sr.ht> wrote: > > From: Suraj Shirvankar <surajshirvankar@gmail.com> > > Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com> Hi; thanks for this patch. Mostly it looks good, but I have a couple of review comments; details below. > --- > contrib/elf2dmp/addrspace.c | 4 ++-- > contrib/elf2dmp/main.c | 4 ++-- > contrib/elf2dmp/pdb.c | 4 ++-- > contrib/elf2dmp/qemu_elf.c | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c > index 64b5d680ad..3bfbb5093c 100644 > --- a/contrib/elf2dmp/addrspace.c > +++ b/contrib/elf2dmp/addrspace.c > @@ -72,7 +72,7 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf) > } > } > > - ps->block = malloc(sizeof(*ps->block) * ps->block_nr); > + ps->block = g_new(struct pa_block, ps->block_nr); > if (!ps->block) { > return 1; > } Unlike malloc(), g_new() can never fail. So the error check for NULL becomes redundant, and we can remove it. Similarly in the other cases below (including the g_malloc() call). > @@ -97,7 +97,7 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf) > void pa_space_destroy(struct pa_space *ps) > { > ps->block_nr = 0; > - free(ps->block); > + g_free(ps->block); > } > > void va_space_set_dtb(struct va_space *vs, uint64_t dtb) > diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c > index 5db163bdbe..97baf0c0c1 100644 > --- a/contrib/elf2dmp/main.c > +++ b/contrib/elf2dmp/main.c > @@ -120,14 +120,14 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t KernBase, struct pdb_reader *pdb, > } > } > > - kdbg = malloc(kdbg_hdr.Size); > + kdbg = g_malloc(kdbg_hdr.Size); > if (!kdbg) { > return NULL; > } > > if (va_space_rw(vs, KdDebuggerDataBlock, kdbg, kdbg_hdr.Size, 0)) { > eprintf("Failed to extract entire KDBG\n"); > - free(kdbg); > + g_free(kdbg); > return NULL; > } This isn't the only place where we free the memory we allocate here. The other place is the "free(kdbg)" at the bottom of the main() function. So for consistency we should change that also to g_free(). > diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c > index 6ca5086f02..625001d1cf 100644 > --- a/contrib/elf2dmp/pdb.c > +++ b/contrib/elf2dmp/pdb.c > @@ -116,7 +116,7 @@ static void *pdb_ds_read(const PDB_DS_HEADER *header, > > nBlocks = (size + header->block_size - 1) / header->block_size; > > - buffer = malloc(nBlocks * header->block_size); > + buffer = g_malloc(nBlocks * header->block_size); > if (!buffer) { > return NULL; > } Similarly here the buffer we allocated is usually returned from this function, assigned to some struct field, and then free()d much later on. So we should also switch all the other free() calls in this file to g_free(). We should end up with no calls to free() left at all in the contrib/elf2dmp/ source files, I think. > @@ -201,7 +201,7 @@ static int pdb_init_symbols(struct pdb_reader *r) > return 0; > > out_symbols: > - free(symbols); > + g_free(symbols); > > return err; > } > diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c > index de6ad744c6..9aa8715108 100644 > --- a/contrib/elf2dmp/qemu_elf.c > +++ b/contrib/elf2dmp/qemu_elf.c > @@ -94,7 +94,7 @@ static int init_states(QEMU_Elf *qe) > > printf("%zu CPU states has been found\n", cpu_nr); > > - qe->state = malloc(sizeof(*qe->state) * cpu_nr); > + qe->state = g_new(QEMUCPUState*, cpu_nr); > if (!qe->state) { > return 1; > } > @@ -115,7 +115,7 @@ static int init_states(QEMU_Elf *qe) > > static void exit_states(QEMU_Elf *qe) > { > - free(qe->state); > + g_free(qe->state); > } > > static bool check_ehdr(QEMU_Elf *qe) > -- > 2.38.5 thanks -- PMM
Thanks for review.My bad I forgot to change the remaining frees.Going to send another patch On Mon, Oct 16, 2023 at 6:49 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Tue, 3 Oct 2023 at 18:23, ~h0lyalg0rithm <h0lyalg0rithm@git.sr.ht> > wrote: > > > > From: Suraj Shirvankar <surajshirvankar@gmail.com> > > > > Signed-off-by: Suraj Shirvankar <surajshirvankar@gmail.com> > > Hi; thanks for this patch. Mostly it looks good, but I > have a couple of review comments; details below. > > > --- > > contrib/elf2dmp/addrspace.c | 4 ++-- > > contrib/elf2dmp/main.c | 4 ++-- > > contrib/elf2dmp/pdb.c | 4 ++-- > > contrib/elf2dmp/qemu_elf.c | 4 ++-- > > 4 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c > > index 64b5d680ad..3bfbb5093c 100644 > > --- a/contrib/elf2dmp/addrspace.c > > +++ b/contrib/elf2dmp/addrspace.c > > @@ -72,7 +72,7 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf > *qemu_elf) > > } > > } > > > > - ps->block = malloc(sizeof(*ps->block) * ps->block_nr); > > + ps->block = g_new(struct pa_block, ps->block_nr); > > if (!ps->block) { > > return 1; > > } > > Unlike malloc(), g_new() can never fail. So the error check > for NULL becomes redundant, and we can remove it. Similarly > in the other cases below (including the g_malloc() call). > > > @@ -97,7 +97,7 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf > *qemu_elf) > > void pa_space_destroy(struct pa_space *ps) > > { > > ps->block_nr = 0; > > - free(ps->block); > > + g_free(ps->block); > > } > > > > void va_space_set_dtb(struct va_space *vs, uint64_t dtb) > > diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c > > index 5db163bdbe..97baf0c0c1 100644 > > --- a/contrib/elf2dmp/main.c > > +++ b/contrib/elf2dmp/main.c > > @@ -120,14 +120,14 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t > KernBase, struct pdb_reader *pdb, > > } > > } > > > > - kdbg = malloc(kdbg_hdr.Size); > > + kdbg = g_malloc(kdbg_hdr.Size); > > if (!kdbg) { > > return NULL; > > } > > > > if (va_space_rw(vs, KdDebuggerDataBlock, kdbg, kdbg_hdr.Size, 0)) { > > eprintf("Failed to extract entire KDBG\n"); > > - free(kdbg); > > + g_free(kdbg); > > return NULL; > > } > > This isn't the only place where we free the memory we > allocate here. The other place is the "free(kdbg)" at the > bottom of the main() function. So for consistency we should > change that also to g_free(). > > > diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c > > index 6ca5086f02..625001d1cf 100644 > > --- a/contrib/elf2dmp/pdb.c > > +++ b/contrib/elf2dmp/pdb.c > > @@ -116,7 +116,7 @@ static void *pdb_ds_read(const PDB_DS_HEADER *header, > > > > nBlocks = (size + header->block_size - 1) / header->block_size; > > > > - buffer = malloc(nBlocks * header->block_size); > > + buffer = g_malloc(nBlocks * header->block_size); > > if (!buffer) { > > return NULL; > > } > > Similarly here the buffer we allocated is usually returned > from this function, assigned to some struct field, and then > free()d much later on. So we should also switch all the other > free() calls in this file to g_free(). > > We should end up with no calls to free() left at all in > the contrib/elf2dmp/ source files, I think. > > > @@ -201,7 +201,7 @@ static int pdb_init_symbols(struct pdb_reader *r) > > return 0; > > > > out_symbols: > > - free(symbols); > > + g_free(symbols); > > > > return err; > > } > > diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c > > index de6ad744c6..9aa8715108 100644 > > --- a/contrib/elf2dmp/qemu_elf.c > > +++ b/contrib/elf2dmp/qemu_elf.c > > @@ -94,7 +94,7 @@ static int init_states(QEMU_Elf *qe) > > > > printf("%zu CPU states has been found\n", cpu_nr); > > > > - qe->state = malloc(sizeof(*qe->state) * cpu_nr); > > + qe->state = g_new(QEMUCPUState*, cpu_nr); > > if (!qe->state) { > > return 1; > > } > > @@ -115,7 +115,7 @@ static int init_states(QEMU_Elf *qe) > > > > static void exit_states(QEMU_Elf *qe) > > { > > - free(qe->state); > > + g_free(qe->state); > > } > > > > static bool check_ehdr(QEMU_Elf *qe) > > -- > > 2.38.5 > > thanks > -- PMM >
diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c index 64b5d680ad..3bfbb5093c 100644 --- a/contrib/elf2dmp/addrspace.c +++ b/contrib/elf2dmp/addrspace.c @@ -72,7 +72,7 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf) } } - ps->block = malloc(sizeof(*ps->block) * ps->block_nr); + ps->block = g_new(struct pa_block, ps->block_nr); if (!ps->block) { return 1; } @@ -97,7 +97,7 @@ int pa_space_create(struct pa_space *ps, QEMU_Elf *qemu_elf) void pa_space_destroy(struct pa_space *ps) { ps->block_nr = 0; - free(ps->block); + g_free(ps->block); } void va_space_set_dtb(struct va_space *vs, uint64_t dtb) diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c index 5db163bdbe..97baf0c0c1 100644 --- a/contrib/elf2dmp/main.c +++ b/contrib/elf2dmp/main.c @@ -120,14 +120,14 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t KernBase, struct pdb_reader *pdb, } } - kdbg = malloc(kdbg_hdr.Size); + kdbg = g_malloc(kdbg_hdr.Size); if (!kdbg) { return NULL; } if (va_space_rw(vs, KdDebuggerDataBlock, kdbg, kdbg_hdr.Size, 0)) { eprintf("Failed to extract entire KDBG\n"); - free(kdbg); + g_free(kdbg); return NULL; } diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c index 6ca5086f02..625001d1cf 100644 --- a/contrib/elf2dmp/pdb.c +++ b/contrib/elf2dmp/pdb.c @@ -116,7 +116,7 @@ static void *pdb_ds_read(const PDB_DS_HEADER *header, nBlocks = (size + header->block_size - 1) / header->block_size; - buffer = malloc(nBlocks * header->block_size); + buffer = g_malloc(nBlocks * header->block_size); if (!buffer) { return NULL; } @@ -201,7 +201,7 @@ static int pdb_init_symbols(struct pdb_reader *r) return 0; out_symbols: - free(symbols); + g_free(symbols); return err; } diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c index de6ad744c6..9aa8715108 100644 --- a/contrib/elf2dmp/qemu_elf.c +++ b/contrib/elf2dmp/qemu_elf.c @@ -94,7 +94,7 @@ static int init_states(QEMU_Elf *qe) printf("%zu CPU states has been found\n", cpu_nr); - qe->state = malloc(sizeof(*qe->state) * cpu_nr); + qe->state = g_new(QEMUCPUState*, cpu_nr); if (!qe->state) { return 1; } @@ -115,7 +115,7 @@ static int init_states(QEMU_Elf *qe) static void exit_states(QEMU_Elf *qe) { - free(qe->state); + g_free(qe->state); } static bool check_ehdr(QEMU_Elf *qe)