[v2,07/18] hw/nvram/fw_cfg: Add fw_cfg_common_unrealize()

Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there
was no QOM design, object where not created and released at runtime.
Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
adding the fw_cfg_common_realize() method.
The time has come to add the equivalent destructor and release the
memory allocated for 'files'.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/nvram/fw_cfg.c | 9 +++++++++
 1 file changed, 9 insertions(+)

On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
> Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there
> was no QOM design, object where not created and released at runtime.
> Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
> adding the fw_cfg_common_realize() method.
> The time has come to add the equivalent destructor and release the
> memory allocated for 'files'.

You should mention that the unrealize function is currently never called
since the object never gets destroyed (AFAIK). But I hope we can fix
that in the not so distant future, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

On 03/08/19 07:55, Thomas Huth wrote:
> On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
>> Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there
>> was no QOM design, object where not created and released at runtime.

s/object where/objects were/

>> Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
>> adding the fw_cfg_common_realize() method.

(I'm not sure if 38f3adc34d "finished the QOM conversion", but that's
just my lack of QOM knowledge. Hopefully someone can confirm whether
this statement is accurate.)

>> The time has come to add the equivalent destructor and release the
>> memory allocated for 'files'.
> 
> You should mention that the unrealize function is currently never called
> since the object never gets destroyed (AFAIK). But I hope we can fix
> that in the not so distant future, so:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

How about we delay this patch until such time the function is actually
called?

This series is already huge, and quite divergent considering its goals.
(Please refer to the blurb.)

I don't mind this patch, but I think it should either belong to a
separate fw_cfg cleanup series (or "wave" if you like).

Or else, we should have a bug reported somewhere public, ensuring that
we eventually call the function introduced here. And then the commit
message should spell out -- as you say -- that the function is not
called yet, but it will be, because of <ticket-reference>.

Thanks
Laszlo

Laszlo Ersek <lersek@redhat.com> writes:

> On 03/08/19 07:55, Thomas Huth wrote:
>> On 08/03/2019 02.32, Philippe Mathieu-Daudé wrote:
>>> Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there

Suggest to say "commit abe147e0ce4".

>>> was no QOM design, object where not created and released at runtime.
>
> s/object where/objects were/
>
>>> Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
>>> adding the fw_cfg_common_realize() method.
>
> (I'm not sure if 38f3adc34d "finished the QOM conversion", but that's
> just my lack of QOM knowledge. Hopefully someone can confirm whether
> this statement is accurate.)
>
>>> The time has come to add the equivalent destructor and release the
>>> memory allocated for 'files'.
>> 
>> You should mention that the unrealize function is currently never called
>> since the object never gets destroyed (AFAIK). But I hope we can fix
>> that in the not so distant future, so:
>> 
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> 
>
> How about we delay this patch until such time the function is actually
> called?
>
> This series is already huge, and quite divergent considering its goals.
> (Please refer to the blurb.)
>
> I don't mind this patch, but I think it should either belong to a
> separate fw_cfg cleanup series (or "wave" if you like).
>
> Or else, we should have a bug reported somewhere public, ensuring that
> we eventually call the function introduced here. And then the commit
> message should spell out -- as you say -- that the function is not
> called yet, but it will be, because of <ticket-reference>.

I suspect filing a bug "somewhere" and making use of it would be much
more work than simply applying the fix.

I'm fine with including trivial patches for stuff you spot "on the way".
I'd be also fine with some patches spun off into a separate cleanup
series that László doesn't have to review, if you can do that safely,
and with little effort.

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Back in abe147e0ce4 when fw_cfg_add_file() was introduced, there
> was no QOM design, object where not created and released at runtime.
> Later 38f3adc34d finished the QOM conversion of the fw_cfg device,
> adding the fw_cfg_common_realize() method.
> The time has come to add the equivalent destructor and release the
> memory allocated for 'files'.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/nvram/fw_cfg.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index b2dc0a80cb..0fb020edce 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -959,6 +959,13 @@ static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
>      qemu_add_machine_init_done_notifier(&s->machine_ready);
>  }
>  
> +static void fw_cfg_common_unrealize(DeviceState *dev, Error **errp)
> +{
> +    FWCfgState *s = FW_CFG(dev);
> +
> +    g_free(s->files);
> +}
> +

Still leaks at least s->entries[0], s->entries[1], s->entry_order,
doesn't it?

>  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>                                  AddressSpace *dma_as)
>  {
> @@ -1127,6 +1134,7 @@ static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = fw_cfg_io_realize;
> +    dc->unrealize = fw_cfg_common_unrealize;
>      dc->props = fw_cfg_io_properties;
>  }
>  
> @@ -1190,6 +1198,7 @@ static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = fw_cfg_mem_realize;
> +    dc->unrealize = fw_cfg_common_unrealize;
>      dc->props = fw_cfg_mem_properties;
>  }