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

[Laszlo Ersek <lersek@redhat.com>]

On 12/16/14 18:20, Alexander Graf wrote:
> 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.

Okay, I understand it now. Thanks.

Laszlo