Re: [Qemu-trivial] [PATCH] hw/nvram/fw_cfg: Move boot_splash_filedata variables into fw_cfg.c

[Thomas Huth]

On 25/02/2019 19.31, Laszlo Ersek wrote:
> Hi Thomas,
> 
> On 02/25/19 17:14, Thomas Huth wrote:
>> The global boot_splash_filedata and boot_splash_filedata_size variables
>> are only used in fw_cfg.c. So there is really no need that these need
>> to be global and reside in vl.c. Move them to fw_cfg.c instead.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  hw/nvram/fw_cfg.c         |  9 +++++++++
>>  include/hw/nvram/fw_cfg.h |  1 +
>>  include/sysemu/sysemu.h   |  2 --
>>  vl.c                      | 10 +---------
>>  4 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 7fdf04a..15f0023 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -63,6 +63,9 @@ struct FWCfgEntry {
>>  #define JPG_FILE 0
>>  #define BMP_FILE 1
>>  
>> +static uint8_t *boot_splash_filedata;
>> +static size_t boot_splash_filedata_size;
>> +
>>  static char *read_splashfile(char *filename, gsize *file_sizep,
>>                               int *file_typep)
>>  {
>> @@ -175,6 +178,12 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>>      }
>>  }
>>  
>> +void fw_cfg_res_free(void)
>> +{
>> +    g_free(boot_splash_filedata);
>> +    boot_splash_filedata = NULL;
>> +}
>> +
>>  static void fw_cfg_reboot(FWCfgState *s)
>>  {
>>      const char *reboot_timeout = NULL;
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index f5a6895..16d237c 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -225,5 +225,6 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>>  
>>  FWCfgState *fw_cfg_find(void);
>>  bool fw_cfg_dma_enabled(void *opaque);
>> +void fw_cfg_res_free(void);
>>  
>>  #endif
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 4b5a6b7..63a5eed 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -111,8 +111,6 @@ extern int no_shutdown;
>>  extern int old_param;
>>  extern int boot_menu;
>>  extern bool boot_strict;
>> -extern uint8_t *boot_splash_filedata;
>> -extern size_t boot_splash_filedata_size;
>>  extern bool enable_mlock;
>>  extern bool enable_cpu_pm;
>>  extern QEMUClockType rtc_clock;
>> diff --git a/vl.c b/vl.c
>> index 502857a..f769cce 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -187,8 +187,6 @@ unsigned int nb_prom_envs = 0;
>>  const char *prom_envs[MAX_PROM_ENVS];
>>  int boot_menu;
>>  bool boot_strict;
>> -uint8_t *boot_splash_filedata;
>> -size_t boot_splash_filedata_size;
>>  bool wakeup_suspend_enabled;
>>  
>>  int icount_align_option;
>> @@ -559,12 +557,6 @@ const char *qemu_get_vm_name(void)
>>      return qemu_name;
>>  }
>>  
>> -static void res_free(void)
>> -{
>> -    g_free(boot_splash_filedata);
>> -    boot_splash_filedata = NULL;
>> -}
>> -
>>  static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
>>  {
>>      const char *driver = qemu_opt_get(opts, "driver");
>> @@ -4585,7 +4577,7 @@ int main(int argc, char **argv, char **envp)
>>      job_cancel_sync_all();
>>      bdrv_close_all();
>>  
>> -    res_free();
>> +    fw_cfg_res_free();
>>  
>>      /* vhost-user must be cleaned up before chardevs.  */
>>      tpm_cleanup();
>>
> 
> The res_free() function call in main() goes back to commit 3d3b8303c6f8
> ("showing a splash picture when start", 2011-07-29). That seems to be
> the original addition of the bootsplash feature. And, res_free() appears
> to simply release dynamic memory before the QEMU process exits.
> 
> Do we release malloc'd memory, as a general rule, before we exit the
> QEMU process (with exit status 0)? My impression has been that we don't
> care; we just let the kernel forget about all memory that the QEMU
> process used to own.
> 
> If that's the case, I suggest to respin this patch, deleting res_free()
> completely. AFAICS there are no other call sites. That would be a nice
> improvement IMO (we wouldn't have to extend "include/hw/nvram/fw_cfg.h").

Hmm, thinking about this for a while, the fw_cfg is a qdev device, so I
think we should at least add an unrealize() function where we clean up
the allocated memory, even if the unrealize function is currently not
called yet, since the device never gets destroyed...

 Thomas