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 <ler...@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
