The next patch will need to free a rom.  There is already code to do
this in rom_add_file().

Note that rom_add_file() uses:

  rom = g_malloc0(sizeof(*rom));
  ...
  if (rom->fw_dir) {
      g_free(rom->fw_dir);
      g_free(rom->fw_file);
  }

The conditional is unnecessary since g_free(NULL) is a no-op.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/core/loader.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index bbb6e65bb5..0c72e7c05a 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -847,6 +847,17 @@ struct Rom {
 static FWCfgState *fw_cfg;
 static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
 
+/* rom->data must be heap-allocated (do not use with rom_add_elf_program()) */
+static void rom_free(Rom *rom)
+{
+    g_free(rom->data);
+    g_free(rom->path);
+    g_free(rom->name);
+    g_free(rom->fw_dir);
+    g_free(rom->fw_file);
+    g_free(rom);
+}
+
 static inline bool rom_order_compare(Rom *rom, Rom *item)
 {
     return ((uintptr_t)(void *)rom->as > (uintptr_t)(void *)item->as) ||
@@ -995,15 +1006,7 @@ err:
     if (fd != -1)
         close(fd);
 
-    g_free(rom->data);
-    g_free(rom->path);
-    g_free(rom->name);
-    if (fw_dir) {
-        g_free(rom->fw_dir);
-        g_free(rom->fw_file);
-    }
-    g_free(rom);
-
+    rom_free(rom);
     return -1;
 }
 
-- 
2.17.1

On Fri, Aug 3, 2018 at 7:47 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The next patch will need to free a rom.  There is already code to do
> this in rom_add_file().
>
> Note that rom_add_file() uses:
>
>   rom = g_malloc0(sizeof(*rom));
>   ...
>   if (rom->fw_dir) {
>       g_free(rom->fw_dir);
>       g_free(rom->fw_file);
>   }
>
> The conditional is unnecessary since g_free(NULL) is a no-op.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/core/loader.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index bbb6e65bb5..0c72e7c05a 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -847,6 +847,17 @@ struct Rom {
>  static FWCfgState *fw_cfg;
>  static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
>
> +/* rom->data must be heap-allocated (do not use with rom_add_elf_program()) */
> +static void rom_free(Rom *rom)
> +{
> +    g_free(rom->data);
> +    g_free(rom->path);
> +    g_free(rom->name);
> +    g_free(rom->fw_dir);
> +    g_free(rom->fw_file);
> +    g_free(rom);
> +}
> +
>  static inline bool rom_order_compare(Rom *rom, Rom *item)
>  {
>      return ((uintptr_t)(void *)rom->as > (uintptr_t)(void *)item->as) ||
> @@ -995,15 +1006,7 @@ err:
>      if (fd != -1)
>          close(fd);
>
> -    g_free(rom->data);
> -    g_free(rom->path);
> -    g_free(rom->name);
> -    if (fw_dir) {
> -        g_free(rom->fw_dir);
> -        g_free(rom->fw_file);
> -    }
> -    g_free(rom);
> -
> +    rom_free(rom);
>      return -1;
>  }
>
> --
> 2.17.1
>
>

On 08/03/2018 11:47 AM, Stefan Hajnoczi wrote:
> The next patch will need to free a rom.  There is already code to do
> this in rom_add_file().
> 
> Note that rom_add_file() uses:
> 
>   rom = g_malloc0(sizeof(*rom));
>   ...
>   if (rom->fw_dir) {
>       g_free(rom->fw_dir);
>       g_free(rom->fw_file);
>   }
> 
> The conditional is unnecessary since g_free(NULL) is a no-op.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/core/loader.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index bbb6e65bb5..0c72e7c05a 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -847,6 +847,17 @@ struct Rom {
>  static FWCfgState *fw_cfg;
>  static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
>  
> +/* rom->data must be heap-allocated (do not use with rom_add_elf_program()) */
> +static void rom_free(Rom *rom)
> +{
> +    g_free(rom->data);
> +    g_free(rom->path);
> +    g_free(rom->name);
> +    g_free(rom->fw_dir);
> +    g_free(rom->fw_file);
> +    g_free(rom);
> +}
> +
>  static inline bool rom_order_compare(Rom *rom, Rom *item)
>  {
>      return ((uintptr_t)(void *)rom->as > (uintptr_t)(void *)item->as) ||
> @@ -995,15 +1006,7 @@ err:
>      if (fd != -1)
>          close(fd);
>  
> -    g_free(rom->data);
> -    g_free(rom->path);
> -    g_free(rom->name);
> -    if (fw_dir) {
> -        g_free(rom->fw_dir);
> -        g_free(rom->fw_file);
> -    }
> -    g_free(rom);
> -
> +    rom_free(rom);
>      return -1;
>  }
>  
>