From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55488) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0vo3-0005mi-8z for qemu-devel@nongnu.org; Tue, 16 Dec 2014 12:21:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0vnv-0003U0-QK for qemu-devel@nongnu.org; Tue, 16 Dec 2014 12:21:07 -0500 Received: from cantor2.suse.de ([195.135.220.15]:52939 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0vnv-0003Tk-K8 for qemu-devel@nongnu.org; Tue, 16 Dec 2014 12:20:59 -0500 Message-ID: <549069F7.5030907@suse.de> Date: Tue, 16 Dec 2014 18:20:55 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1418399932-7658-1-git-send-email-lersek@redhat.com> <1418399932-7658-4-git-send-email-lersek@redhat.com> <5490203B.2030001@suse.de> <549028B2.9080506@redhat.com> <549064DD.7060707@redhat.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 3/8] fw_cfg: introduce the "data_memwidth" property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , Laszlo Ersek Cc: Andrew Jones , QEMU Developers "Richard W.M. Jones" On 12/16/14 18:10, Peter Maydell wrote: > On 16 December 2014 at 16:59, Laszlo Ersek 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