On Fri, 2007-11-05 at 19:01 +0200, Thomas Graf wrote:
> * jamal <[EMAIL PROTECTED]> 2007-05-10 20:13
> > * Compute the worst case header length according to the protocols
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index f28bb2d..b821040 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -59,7 +59,79 @@ void qdisc_unlock_tree(struct net_device *dev)
> > spin_unlock_bh(&dev->queue_lock);
> > }
> >
> > -/*
> > +static inline int qqlen(struct Qdisc *q)
> > +{
> > + BUG_ON((int) q->q.qlen < 0);
> > + return q->q.qlen;
> > +}
>
> Nitpicking but qdisc_qlen() sounds a lot better to me.
>
I thought nitpicking was my department ;->
qdisc_qlen is more descriptive; i will change it.
> > +static inline int
> > +handle_dev_requeue(struct sk_buff *skb, struct net_device *dev, struct
> > Qdisc *q)
> > +{
> > +
> > + if (unlikely(q == &noop_qdisc))
> > + kfree_skb(skb);
> > +
> > + if (skb->next)
>
> Oops, missing else here, this will crash.
>
tabarnaq. I will fix - thanks.
> Seems simpler to just return the skb or NULL instead
>
sure.
> > + /* churn baby churn .. */
> > + ret = dev_hard_start_xmit(skb, dev);
>
> Check whether queue is stopped got lost here
>
Also caught by Peter - will fix.
> > + q = dev->qdisc;
>
> Maybe comment why q needs to be refreshed so nobody removes
> it by accident.
Makes sense ..
> > + /* most likely result, packet went ok */
> > + if (ret == NETDEV_TX_OK)
> > + return qqlen(q);
> > + /* only for lockless drivers .. */
> > + if (ret == NETDEV_TX_LOCKED && lockless) {
> > + return handle_tx_locked(skb, dev, q);
> > + }
>
> Your style of placing parentheses seems to vary randomly :-)
There may have been a debug in there that i removed. Frankly
i dont mind having braces even for a one liner, but i know that is not
popular with many folks.
Any other spot you noticed?
Thanks a lot Thomas.
cheers,
jamal
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
