| Message ID | 20190308013222.12524-13-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | fw_cfg: reduce memleaks, add QMP/HMP info + edk2_add_host_crypto_policy | expand |
On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote: > The 'file_data' is allocated by read_splashfile() (introduced in > commit 3d3b8303c6f8). It is then used by fw_cfg_add_file(). Due > to the contract interface of fw_cfg_add_file(), it has to be valid > for the lifetime of the FwCfg object. > > Keep a reference of 'file_data' in FWCfgState to be able to > free this memory in fw_cfg_common_unrealize(). > We can now remove the res_free() from the main() loop. > The global boot_splash_filedata is now unused, remove it. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/nvram/fw_cfg.c | 10 ++++++---- > include/hw/nvram/fw_cfg.h | 1 + > include/sysemu/sysemu.h | 1 - > vl.c | 9 --------- > 4 files changed, 7 insertions(+), 14 deletions(-) Reviewed-by: Thomas Huth <thuth@redhat.com>
On 03/08/19 02:32, Philippe Mathieu-Daudé wrote: > The 'file_data' is allocated by read_splashfile() (introduced in > commit 3d3b8303c6f8). It is then used by fw_cfg_add_file(). Due > to the contract interface of fw_cfg_add_file(), it has to be valid > for the lifetime of the FwCfg object. > > Keep a reference of 'file_data' in FWCfgState to be able to > free this memory in fw_cfg_common_unrealize(). > We can now remove the res_free() from the main() loop. > The global boot_splash_filedata is now unused, remove it. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/nvram/fw_cfg.c | 10 ++++++---- > include/hw/nvram/fw_cfg.h | 1 + > include/sysemu/sysemu.h | 1 - > vl.c | 9 --------- > 4 files changed, 7 insertions(+), 14 deletions(-) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 3ac6687a04..fc392cb7e0 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -215,16 +215,16 @@ static void fw_cfg_bootsplash(FWCfgState *s) > g_free(filename); > return; > } > - g_free(boot_splash_filedata); > - boot_splash_filedata = (uint8_t *)file_data; > + g_free(s->boot_splash.file_data); > + s->boot_splash.file_data = file_data; > > /* insert data */ > if (file_type == JPG_FILE) { > fw_cfg_add_file(s, "bootsplash.jpg", > - boot_splash_filedata, file_size); > + s->boot_splash.file_data, file_size); > } else { > fw_cfg_add_file(s, "bootsplash.bmp", > - boot_splash_filedata, file_size); > + s->boot_splash.file_data, file_size); > } > g_free(filename); > } > @@ -974,6 +974,8 @@ static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp) > > g_free(s->files); > > + g_free(s->boot_splash.file_data); > + > g_free(s->entries[0]); > g_free(s->entries[1]); > g_free(s->entry_order); > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index fcb771186c..83a0540b6c 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -56,6 +56,7 @@ struct FWCfgState { > > uint32_t reboot_timeout; > struct { > + char *file_data; > uint16_t time_le16; > } boot_splash; > }; > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index 6065d9e420..3cd856b015 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -109,7 +109,6 @@ extern int no_shutdown; > extern int old_param; > extern int boot_menu; > extern bool boot_strict; > -extern uint8_t *boot_splash_filedata; > extern bool enable_mlock; > extern bool enable_cpu_pm; > extern QEMUClockType rtc_clock; > diff --git a/vl.c b/vl.c > index fad6fec38c..47dd63a309 100644 > --- a/vl.c > +++ b/vl.c > @@ -187,7 +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; > bool wakeup_suspend_enabled; > > int icount_align_option; > @@ -558,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"); > @@ -4591,8 +4584,6 @@ int main(int argc, char **argv, char **envp) > job_cancel_sync_all(); > bdrv_close_all(); > > - res_free(); > - > /* vhost-user must be cleaned up before chardevs. */ > tpm_cleanup(); > net_cleanup(); > Referring to the earlier thread [Qemu-devel] [PATCH] hw/nvram/fw_cfg: Move boot_splash_filedata variables into fw_cfg.c https://www.mail-archive.com/qemu-devel@nongnu.org/msg599282.html my opinion is that *all* of the g_free() calls touched in this patch are presently -- that is, pre-patch -- bogus: - As I wrote earlier, res_free() may be reached, but the freeing it does is useless. - Furthermore, the g_free() call in fw_cfg_bootsplash() never frees anything in reality. It is only called from fw_cfg_common_realize(), and we only have one fw_cfg object (which is never destructed, for now). So, first I would kill these bogus g_free()s altogether, in a separate patch (in the separate series that I've recommended elsewhere). Then, in a second patch (in the separate series), I would include the boot splash image among the dynamically tracked allocations. Just add it to a linked list, and when the fw_cfg object is destroyed, release it with the rest. (Another note (and I should have made it earlier): fw_cfg_modify_i16() will face a challenge; it will have to update the tracker data structure too.) Thanks Laszlo
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 3ac6687a04..fc392cb7e0 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -215,16 +215,16 @@ static void fw_cfg_bootsplash(FWCfgState *s) g_free(filename); return; } - g_free(boot_splash_filedata); - boot_splash_filedata = (uint8_t *)file_data; + g_free(s->boot_splash.file_data); + s->boot_splash.file_data = file_data; /* insert data */ if (file_type == JPG_FILE) { fw_cfg_add_file(s, "bootsplash.jpg", - boot_splash_filedata, file_size); + s->boot_splash.file_data, file_size); } else { fw_cfg_add_file(s, "bootsplash.bmp", - boot_splash_filedata, file_size); + s->boot_splash.file_data, file_size); } g_free(filename); } @@ -974,6 +974,8 @@ static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp) g_free(s->files); + g_free(s->boot_splash.file_data); + g_free(s->entries[0]); g_free(s->entries[1]); g_free(s->entry_order); diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h index fcb771186c..83a0540b6c 100644 --- a/include/hw/nvram/fw_cfg.h +++ b/include/hw/nvram/fw_cfg.h @@ -56,6 +56,7 @@ struct FWCfgState { uint32_t reboot_timeout; struct { + char *file_data; uint16_t time_le16; } boot_splash; }; diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 6065d9e420..3cd856b015 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -109,7 +109,6 @@ extern int no_shutdown; extern int old_param; extern int boot_menu; extern bool boot_strict; -extern uint8_t *boot_splash_filedata; extern bool enable_mlock; extern bool enable_cpu_pm; extern QEMUClockType rtc_clock; diff --git a/vl.c b/vl.c index fad6fec38c..47dd63a309 100644 --- a/vl.c +++ b/vl.c @@ -187,7 +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; bool wakeup_suspend_enabled; int icount_align_option; @@ -558,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"); @@ -4591,8 +4584,6 @@ int main(int argc, char **argv, char **envp) job_cancel_sync_all(); bdrv_close_all(); - res_free(); - /* vhost-user must be cleaned up before chardevs. */ tpm_cleanup(); net_cleanup();
The 'file_data' is allocated by read_splashfile() (introduced in commit 3d3b8303c6f8). It is then used by fw_cfg_add_file(). Due to the contract interface of fw_cfg_add_file(), it has to be valid for the lifetime of the FwCfg object. Keep a reference of 'file_data' in FWCfgState to be able to free this memory in fw_cfg_common_unrealize(). We can now remove the res_free() from the main() loop. The global boot_splash_filedata is now unused, remove it. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/nvram/fw_cfg.c | 10 ++++++---- include/hw/nvram/fw_cfg.h | 1 + include/sysemu/sysemu.h | 1 - vl.c | 9 --------- 4 files changed, 7 insertions(+), 14 deletions(-)