Re: [Qemu-devel] [PATCH v4 3/8] fw_cfg: introduce the "data_memwidth" property

[Alexander Graf <agraf@suse.de>]

On 12/16/14 18:10, Peter Maydell wrote:
> On 16 December 2014 at 16:59, Laszlo Ersek <lersek@redhat.com> wrote:
>> To elaborate on the above -- the fw_cfg device appears to be
>> undestructible at the moment. It has no unrealize callback. If it were
>> destructible, then the above leak would be the smallest of concerns --
>> it doesn't unmap nor destroy the memory regions that implement the
>> various registers.
>>
>> So, I think the above is not an actual leak, because the result of
>> g_memdup() can never become unreferenced.
> True, and we have a lot of device that are in this same
> category of "can't ever be destroyed". However it is setting
> up a minor beartrap for ourselves in future if we have
> allocations which aren't tracked via a field in the device's
> state structure, because the obvious future implementation of
> destruction for a device is "just free/destroy everything
> that is in the state struct".
>
> NB: I think what Alex had in mind with his option (2) was
> just to have a "MemoryRegionOps ops;" field in the state
> struct, and then use "s->ops = data_mem_ops;" rather than
> the memdup. That retains the use of the static field for
> the non-variable-width case, it's just that instead of
> allocating off the heap for the var-width setup we use
> an inline lump of memory in a struct we're already allocing.
>
> I don't think I care very much about this, but Alex's
> suggestion 2 is slightly nicer I guess. Adding a whole
> unrealize callback is definitely vastly overkill.

Yeah, it's exactly what I meant. Sorry for not being as clear. By moving 
the dynamically created struct into the device struct we're just making 
the whole allocation flow easier.

But if this is the only nitpick, there's no need for a respin just for that.


Alex