close tapfd before running down_script [was Re: [Qemu-devel] ANNOUNCE: Release 0.11.0-rc2 of QEMU]

[Juergen Lock]

On Sun, Sep 06, 2009 at 01:50:25PM +0100, Mark McLoughlin wrote:
> On Fri, 2009-09-04 at 22:13 +0200, Juergen Lock wrote:
> 
> >  The second change is a small patch to tap_cleanup that makes it close
> > the tap fd before calling the ifdown script instead of after, otherwise
> > FreeBSD's tap driver may hit a KASSERT in case the ifdown script does
> > something like an `ifconfig tap0 destroy'...
> > 
> > Index: qemu/net.c
> > @@ -1643,12 +1643,13 @@ static void tap_cleanup(VLANClientState 
> >  
> >      qemu_purge_queued_packets(vc);
> >  
> > -    if (s->down_script[0])
> > -        launch_script(s->down_script, s->down_script_arg, s->fd);
> > -
> >      tap_read_poll(s, 0);
> >      tap_write_poll(s, 0);
> >      close(s->fd);
> > +
> > +    if (s->down_script[0])
> > +        launch_script(s->down_script, s->down_script_arg, -1);
> > +
> >      qemu_free(s);
> >  }
> >  
> >  I don't know if there are use cases where the ifdown script needs the
> > tap fd still open, otherwise I guess this can also be committed upstream.
> > And in case you want to: :)
> > 
> >  Signed-off-by: Juergen Lock <nox at jelal.kn-bremen.de>
> 
> I don't ever use the the down script myself, but a couple of things to
> bear in mind:
> 
>   a) 0.9.1 never actually closed the tap fd and since 0.10.0 we've 
>      been closing the fd after calling the script
> 
>   b) where qemu creates the tap interface, by closing the tap fd before 
>      the script we'd be destroying the interface before passing the 
>      interface name to the script
> 
Ah, then that sounds like a difference between Linux and FreeBSD,
on FreeBSD the tap interface continues to exist after closing the fd...
(which is why these scripts call `ifconfig tapX destroy' in the
first place.)

> The current behaviour seems right to me. Could you explain your use case
> a bit more? Maybe post the up and down scripts?

 Here's the thread that started this:
	http://lists.freebsd.org/pipermail/freebsd-emulation/2009-August/006700.html

 And here is FreeBSD's tap_destroy() fn with the mentioned KASSERT:
	http://fxr.watson.org/fxr/source/net/if_tap.c#L213

 Anyway, sounds like the patch is the wrong thing to do on Linux
hosts so I guess I'll just keep it private to the FreeBSD port(s)...
(Or should I repost it with an #ifdef __FreeBSD__ etc?)

 Thanx,
	Juergen