> On 20 Nov 2015, at 11:23 PM, Reyk Floeter <r...@openbsd.org> wrote: > > On Fri, Nov 20, 2015 at 03:36:40PM +1000, David Gwynne wrote: >> IFF_OACTIVE means the hardware ring is full, not if it is busy. >> >> perhaps a better check is to see whether there are pending packets >> on the send queue? >> >> i could also argue we dont need the check at all, but this is less >> of a semantic change. >> >> ok? >> > > OK > > I added this check in the initial commit of trunk(4) more than 10 > years ago. I don't remember a particular reason - there wasn't even a > production use yet. I initially wrote trunk to use it for failover on > some firewalls, but it was not going into production before it was > committed to OpenBSD. > > So the reason for the flag might just be a historical one: back in the > days, I heard that 10 years is a long time in IT, there was not much > reference about implementing such a "cloner" correctly. I must have > looked at vlan(4) or bridge(4) and decided that it was the way to do. > I mean, before mpi@, working in the network stack was tough and very > different: you didn't ask, never refered to any documentation, just > relied on the traditions and repeated what was done by the BSD heroes > in the past.
thats exactly right. please dont take this as an accusation of negligence, this is just mopping up some of that inherited code. im upset about my own use of IFQ_POLL() in a bunch of my own drivers, but really i am only guilty of what you described above too. ie, we're not guilty, we were just following best practices at the time. how would you feel if i simply removed the check? dlg > > Reyk > >> Index: if_trunk.c >> =================================================================== >> RCS file: /cvs/src/sys/net/if_trunk.c,v >> retrieving revision 1.124 >> diff -u -p -r1.124 if_trunk.c >> --- if_trunk.c 20 Nov 2015 05:33:54 -0000 1.124 >> +++ if_trunk.c 20 Nov 2015 05:35:07 -0000 >> @@ -296,7 +296,7 @@ trunk_port_create(struct trunk_softc *tr >> return (ENOSPC); >> >> /* New trunk port has to be in an idle state */ >> - if (ifp->if_flags & IFF_OACTIVE) >> + if (!ifq_empty(&ifp->if_snd)) >> return (EBUSY); >> >> /* Check if port has already been associated to a trunk */ > > --
