From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Lu5il-0008V7-Dv for qemu-devel@nongnu.org; Wed, 15 Apr 2009 10:04:11 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Lu5ih-0008Tj-Nq for qemu-devel@nongnu.org; Wed, 15 Apr 2009 10:04:11 -0400 Received: from [199.232.76.173] (port=45277 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Lu5ih-0008Tc-Ia for qemu-devel@nongnu.org; Wed, 15 Apr 2009 10:04:07 -0400 Received: from lizzard.sbs.de ([194.138.37.39]:19195) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Lu5ih-00034p-4U for qemu-devel@nongnu.org; Wed, 15 Apr 2009 10:04:07 -0400 Message-ID: <49E5E953.8060207@siemens.com> Date: Wed, 15 Apr 2009 16:04:03 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20090414172954.15035.73053.stgit@mchn012c.ww002.siemens.net> <20090414172954.15035.35711.stgit@mchn012c.ww002.siemens.net> <1239800979.4431.128.camel@blaa> In-Reply-To: <1239800979.4431.128.camel@blaa> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 2/7] net: Add VLAN client cleanup handler Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark McLoughlin , qemu-devel@nongnu.org Mark McLoughlin wrote: > Hi Jan, > > On Tue, 2009-04-14 at 19:29 +0200, Jan Kiszka wrote: >> Do proper VLAN client cleanup via a callback handler. This fixes >> resource leakage on host_net_remove and allows a generic net_cleanup >> implementation. > > Yep, we need this. > > However, I've got a different version in my queue (see below) - I've > been holding back on posting it until I finished some other patches I'm > working on, but I'll polish it off and post this afternoon. > > Differences from yours include: > > - I've added cleanup code to all the NICs, including adding > unregister_savevm() so that we don't try and save deleted NICs Different issue than I was trying to fix (host net removals), but valid. > > - Rather than adding yet another param to new_vlan_client(), I just > initialize vc->cleanup after creating the client; another patch in > my queue removes all callbacks to new_vlan_client() because as more > are added it just gets terribly unwieldy. Personally, I prefer a function-based API over, well, hacking the structures directly. The driver should not have to poke into its device descriptor. Yes, this breaks existing code each time you add another callback, but this has the effect that you a) normally think twice if you really need it before doing this, b) likely enhance all users, and c) that new users will find prominent information about this (hopefully) useful callback. > > - I remove the io handler on e.g. the tapfd when freeing - otherwise > we'll continue to poll the fd AFAICS Good point. I ran into similar issues lately with the qemu-char code (fixed upstream now). > > - I implement net_cleanup() by calling del_vlan_client() on all clients Yes, probably the better way around. > > Apart from that, the patches are very similar - are you okay with just > waiting for my patch? If we can agree on second point :) and if your patch will show up soon, I will happily rebase the rest of my series on top of it. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux