On 3/8/19 12:04 PM, Laszlo Ersek wrote: > Hi Phil, > > On 03/08/19 02:32, Philippe Mathieu-Daudé wrote: >> Due to the contract interface of fw_cfg_add_file(), the >> 'reboot_timeout' data has to be valid for the lifetime of the >> FwCfg object. For this reason it is copied on the heap with >> memdup(). >> >> The object state, 'FWCfgState', is also meant to be valid during the >> lifetime of the object. >> Move the 'reboot_timeout' in FWCfgState to achieve the same purpose. >> Doing so we avoid a memory leak. >> >> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >> --- >> hw/nvram/fw_cfg.c | 4 +++- >> include/hw/nvram/fw_cfg.h | 2 ++ >> 2 files changed, 5 insertions(+), 1 deletion(-) > > Currently, there is no memory leak. Right now, the leak is theoretical, > and it would depend on the fw_cfg object being actually destroyed.
Actually my first motivation came while using valgrind, there are a bunch of warnings related to the fw_cfg device. This device is not hotpluggable however, and we don't test it in the device-introspect-test. > I think armoring the fw_cfg implementation for such lifetime actions is > valuable. But, that definitely belongs to its own series, in my opinion. > > In the "hw/nvram/fw_cfg.c" file, I count: > > (a) two "specific purpose" g_memdup() calls, namely in > fw_cfg_bootsplash() and in fw_cfg_reboot(); > > (b) one "generic purpose" g_memdup() call, namely in fw_cfg_add_string(); > > (c) two "generic purpose" g_malloc() calls, namely in fw_cfg_add_i16(), > fw_cfg_add_i32(), and fw_cfg_add_i64(). (The one in fw_cfg_modify_i16() > does not matter here because the previous blob is freed in that function.) > > Your series deals with (a), namely with fw_cfg_reboot() in this patch, > and with fw_cfg_bootsplash() in the next one. > > Your series deals with neither (b) nor (c). The I did a PoC of (b) and (c) but it is a more invasive patchset indeed. > fw_cfg_add_(string|i16|i32|i64) functions are called from a bunch of > places however, so if we really intend *not* to leak those copies upon > fw_cfg destruction, then we'll have to track all of them dynamically, in > a list for example. I haven't think of using a list. > (And that necessitates a separate series for this topic even more.) OK. > In turn, once we add dynamic tracking, for those blobs that the > fw_cfg_add_(string|i16|i32|i64) functions allocate internally -- as they > are advertized to do --, then we might as well use the same tracking > infrastructure for (a). In other words, it should not be necessary to > add the specific fields "reboot_timeout" and "boot_splash" to FWCfgState. OK, I'll drop these patches from this series. > > Thanks, > Laszlo > >> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index b73a591eff..182d27f59a 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -250,7 +250,9 @@ static void fw_cfg_reboot(FWCfgState *s) >> } >> } >> >> - fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4); >> + s->reboot_timeout = rt_val; >> + fw_cfg_add_file(s, "etc/boot-fail-wait", >> + &s->reboot_timeout, sizeof(s->reboot_timeout)); >> } >> >> static void fw_cfg_write(FWCfgState *s, uint8_t value) >> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h >> index 828ad9dedc..99f6fafcaa 100644 >> --- a/include/hw/nvram/fw_cfg.h >> +++ b/include/hw/nvram/fw_cfg.h >> @@ -53,6 +53,8 @@ struct FWCfgState { >> dma_addr_t dma_addr; >> AddressSpace *dma_as; >> MemoryRegion dma_iomem; >> + >> + uint32_t reboot_timeout; >> }; >> >> struct FWCfgIoState { >> >