[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v2 10/18] hw/nvram/fw_cfg: Add reboot_timeout to F
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-arm] [PATCH v2 10/18] hw/nvram/fw_cfg: Add reboot_timeout to FWCfgState |
Date: |
Fri, 8 Mar 2019 08:48:35 -0500 |
On Fri, Mar 08, 2019 at 02:32:14AM +0100, 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é <address@hidden>
I don't like adding random per-file state to fw cfg at all.
I don't see a leak. Please add more explanation about this.
And maybe split from this series.
> ---
> hw/nvram/fw_cfg.c | 4 +++-
> include/hw/nvram/fw_cfg.h | 2 ++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> 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 {
> --
> 2.20.1