[Top] [All Lists]

Re: [PATCH PKT_SCHED 0/17]: tc action cleanup + fixes

To: Patrick McHardy <kaber@xxxxxxxxx>
Subject: Re: [PATCH PKT_SCHED 0/17]: tc action cleanup + fixes
From: jamal <hadi@xxxxxxxxxx>
Date: 30 Dec 2004 08:30:00 -0500
Cc: Maillist netdev <netdev@xxxxxxxxxxx>
In-reply-to: <41D3F5EC.9050808@xxxxxxxxx>
Organization: jamalopolous
References: <41D3785F.3040909@xxxxxxxxx> <1104382562.1048.39.camel@xxxxxxxxxxxxxxxx> <41D3F5EC.9050808@xxxxxxxxx>
Reply-to: hadi@xxxxxxxxxx
Sender: netdev-bounce@xxxxxxxxxxx
On Thu, 2004-12-30 at 07:34, Patrick McHardy wrote:
> jamal wrote:
> Patrick,
> > Thanks for this cleanup.
> > 
> > Questions/comments:
> > 
> > 1)compiler or style issue?
> > 
> > You have a few of fixes from
> > 
> > a)
> >  if (..){
> >     single statement here;
> > }
> >  
> > to:
> > if (..)
> >     single statement here;
> > 
> > I always add an extra pair of brace
> > for lazy reasons (in the back of my mind: incase i want to add another
> > statement ;->).
> Just cleanup, I prefer not to waste too many lines. Saving
> space increases readability.

I am going to try and control my fingers ;-> 
They have a brain of their own.

> > b)Other things which i have seen compilers whine about in the past of
> > the form:
> > 
> > a missing cast
> > -       a->priv = (void *) p;
> > +       a->priv = p;
> No need to cast a pointer to void *, except if a->priv
> was of some different type.

So as long as lvalue was void you dont cast? p is certainly not void.

> > 4) Some of those messages are actually still useful and dont really
> > harm to leave around for a little while longer;
> > -       if (tb[TCA_PEDIT_PARMS - 1] == NULL) {
> > -               printk("BUG: tcf_pedit_init called with NULL params\n");
> > 
> > I realize the fixes you have to return -ENOMEN/NOENT etc are an
> > improvement but a little ascii puking wont harm for somebody writting
> > a user space app until we get better netlink error propagation
> > in place.
> Agreed for some messages, but those should be DEBUGs. Anyway,
> I didn't want to judge for every message and possible convert
> it, so I deleted all printks that got replaced by error codes.

the printks are meant to help a little more (and are mostly on the slow
path); when the error propagation for netlink works well, those sorts of
ascii messages will probably be transported back to user space. On any
newer patches I suggest to just keep them.
Heres something else:
Re: [PATCH PKT_SCHED 15/17]: Remove checks for impossible conditions in
pedit action, you say:

>Remove checks for impossible conditions in pedit action.

-       if (p == NULL) {
> -             printk("BUG: tcf_pedit_dump called with NULL params\n");
> -             goto rtattr_failure;
> -     }
> -

You have these type changes all over. These are certainly artifacts of the 
development time, I may have have caught a bug or two via these checks at 
the time. It is highly likely those bugs are fixed in the code merged.
If they happen, however, they are a BUG and the possibility of a bug is
still there ;-> i.e the word "impossible" is too strong a description. 
Having said that:
Is it better to have an oops catch this or have something print on the
console or syslog indicating a bug? This is more a philosphical question
and an answer could be "good practise is to let oops catch it". I am
actually indifferent if those checks go - however if i had caught them
myself i would have put unlikely() around them.

> > I will look closely at one or two of those fixes in the morning;
> > majority look good on first quick scan (most things were needed during
> > development or are artifacts of that period and are safe to rid of now).
> Thanks, I'll continue later today and send another batch
> tonight.

I will wait for you to finish before i start working on the eactions.

So a general comment to all the patches. All look good - I would prefer
a check against size instead of EOPNOTSUPP for the two i pointed at.
And going forward, prefer you leave the printks i had for errors but fix
the return codes to be more meaningful. So only those two i pointed at
with EOPNOTSUPP i am not ACKing (my basic tests will break) - rest Dave
can push in.

Again, thanks.


<Prev in Thread] Current Thread [Next in Thread>