[Qemu-devel] Re: [PATCH 2/7] net: Add VLAN client cleanup handler

[Jan Kiszka]

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