Re: [PATCH qemu 1/1] Switch memory management calls to new coding conventions

[Peter Maydell <peter.maydell@linaro.org>]

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