netdev - Re: [RFC] make qdisc_restart more readable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1178907104.4126.10.camel@localhost>
Date:	Fri, 11 May 2007 14:11:44 -0400
From:	jamal <hadi@...erus.ca>
To:	Thomas Graf <tgraf@...g.ch>
Cc:	Herbert Xu <herbert@...dor.apana.org.au>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [RFC] make qdisc_restart more readable

On Fri, 2007-11-05 at 19:01 +0200, Thomas Graf wrote:
> * jamal <j.hadi123@...il.com> 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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html