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

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

[Patrick McHardy]

Hi Jamal,

here is what I got so far, I'll continue tommorrow. Only compile
tested yet. Please review and comment.

Patrick McHardy:
   o [PKT_SCHED]: Disable broken override bits in pedit action
   o [PKT_SCHED]: Return proper error codes in tcf_pedit_init
   o [PKT_SCHED]: Remove checks for impossible conditions in pedit action
   o [PKT_SCHED]: Clean up pedit action
   o [PKT_SCHED]: Clean up tcf_ipt_init
   o [PKT_SCHED]: Fix missing unlock in ipt action error path
   o [PKT_SCHED]: Remove checks for impossible conditions in ipt action
   o [PKT_SCHED]: Clean up ipt action
   o [PKT_SCHED]: Return -EOPNOTSUPP if gact probability is requested 
but not compiled in
   o [PKT_SCHED]: Return proper error codes in tcf_gact_init
   o [PKT_SCHED]: Remove checks for impossible conditions in gact action
   o [PKT_SCHED: Clean up gact action
   o [PKT_SCHED]: Clean up act_api.c action init path, propagate errors 
properly
   o [PKT_SCHED]: Check TCA_ACT_KIND payload size _before_ copying it
   o [PKT_SCHED]: Remove checks for impossible conditions in act_api.c
   o [PKT_SCHED]: Consistent comparision style in act_api.c
   o [PKT_SCHED]: act_api.c whitespace cleanup
 

Regards
Patrick

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

[jamal]

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 ;->).

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;

or unitialized vars:
-       struct tcf_pedit *p = NULL;
+       struct tcf_pedit *p;
 

2) You are not the first to not like the 
if (constant != variable_here) 

Should be noted that i am dyxlesic and this has saved 
me a few times (I would say the most common errata for me, weird as that
may sound). Dont have a problem with the changes you made
(dont need the protection at this stage;->).

3) Is there any reason in which some cases you fixed things to be:

return_type
functionname() eg
-static int
-gact_net_rand(struct tcf_gact *p) {
+static int gact_net_rand(struct tcf_gact *p)

and in some cases you leave them to be of the form:
return_type functionname()

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.

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).

Again thanks for the good work.

cheers,
jamal

On Wed, 2004-12-29 at 22:39, Patrick McHardy wrote:
> Hi Jamal,
> 
> here is what I got so far, I'll continue tommorrow. Only compile
> tested yet. Please review and comment.
> 
> Patrick McHardy:
>    o [PKT_SCHED]: Disable broken override bits in pedit action
>    o [PKT_SCHED]: Return proper error codes in tcf_pedit_init
>    o [PKT_SCHED]: Remove checks for impossible conditions in pedit action
>    o [PKT_SCHED]: Clean up pedit action
>    o [PKT_SCHED]: Clean up tcf_ipt_init
>    o [PKT_SCHED]: Fix missing unlock in ipt action error path
>    o [PKT_SCHED]: Remove checks for impossible conditions in ipt action
>    o [PKT_SCHED]: Clean up ipt action
>    o [PKT_SCHED]: Return -EOPNOTSUPP if gact probability is requested 
> but not compiled in
>    o [PKT_SCHED]: Return proper error codes in tcf_gact_init
>    o [PKT_SCHED]: Remove checks for impossible conditions in gact action
>    o [PKT_SCHED: Clean up gact action
>    o [PKT_SCHED]: Clean up act_api.c action init path, propagate errors 
> properly
>    o [PKT_SCHED]: Check TCA_ACT_KIND payload size _before_ copying it
>    o [PKT_SCHED]: Remove checks for impossible conditions in act_api.c
>    o [PKT_SCHED]: Consistent comparision style in act_api.c
>    o [PKT_SCHED]: act_api.c whitespace cleanup
>  
> 
> Regards
> Patrick
> 
> 

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

[Patrick McHardy]

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.

> 
> 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.

> 
> or unitialized vars:
> -       struct tcf_pedit *p = NULL;
> +       struct tcf_pedit *p;

p is assigned another value before the first use, so
initializing to NULL is not neccessary.

> 2) You are not the first to not like the 
> if (constant != variable_here) 
> 
> Should be noted that i am dyxlesic and this has saved 
> me a few times (I would say the most common errata for me, weird as that
> may sound). Dont have a problem with the changes you made
> (dont need the protection at this stage;->).

The compiler warns about assignments in comparisions nowadays.

> 3) Is there any reason in which some cases you fixed things to be:
> 
> return_type
> functionname() eg
> -static int
> -gact_net_rand(struct tcf_gact *p) {
> +static int gact_net_rand(struct tcf_gact *p)
> 
> and in some cases you leave them to be of the form:
> return_type functionname()

Just saving empty lines. I didn't try to be consistent with
this, In case I changed it the other way around it's usually
to keep all arguments on one line without exceeding 80 chars.

> 
> 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.

> 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.

Regards
Patrick

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

[jamal]

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.

cheers,
jamal

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

[Patrick McHardy]

jamal wrote:

>>>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.

Exactly.

>>>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.

Ok.

> 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.

Yes, I checked all paths before removing them.

> 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 prefer an Oops because it gives a backtrace, without requiring
additional checks in the code. The other reason I deleted them was
that not all of them printed something on the console, so some
bugs were just quietly ignored. And I didn't want to add more printks :)

> 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.

Thanks. Dave is on holidays until next week, I'll fix them
up until then.

Regards
Patrick

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

[Patrick McHardy]

Patrick McHardy wrote:
> Thanks, I'll continue later today and send another batch
> tonight.

Thinking about it, I think its better to reorganize the
patches by subject. While doing this I'm going to add
back the useful printks in the init paths as DPRINTKs.
I'm going to post the entire reorganized batch next week
when I return home, working with bitkeeper on my crappy
notebook is just to painful :)

Regards
Patrick

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

[jamal]

On Thu, 2004-12-30 at 11:10, Patrick McHardy wrote:

> Thinking about it, I think its better to reorganize the
> patches by subject. While doing this I'm going to add
> back the useful printks in the init paths as DPRINTKs.
> I'm going to post the entire reorganized batch next week
> when I return home, working with bitkeeper on my crappy
> notebook is just to painful :)

[I dont use bitkeeper for religious reasons (i hope i am not
offending anyone)].
Ok, sounds like a good plan (gives me more time to work with the driver
stuff which is getting out of control ;->): I think the patches may have
goten a little messy. Maybe the order should be: you, Thomas with what
he has now then myself with eaction and Thomas with extended matches (I
dont think the order of the last two matters). I will just code against
whatever latest release is and migrate later when all the otehr patches
are in.

cheers,
jamal

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

[Patrick McHardy]

jamal wrote:

> Ok, sounds like a good plan (gives me more time to work with the driver
> stuff which is getting out of control ;->): I think the patches may have
> goten a little messy. Maybe the order should be: you, Thomas with what
> he has now then myself with eaction and Thomas with extended matches (I
> dont think the order of the last two matters). I will just code against
> whatever latest release is and migrate later when all the otehr patches
> are in.

Sounds good too me.

Regards
Patrick

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

[Thomas Graf]

* Patrick McHardy <41D521EA.2090603@trash.net> 2004-12-31 10:54
> jamal wrote:
> 
> >Ok, sounds like a good plan (gives me more time to work with the driver
> >stuff which is getting out of control ;->): I think the patches may have
> >goten a little messy. Maybe the order should be: you, Thomas with what
> >he has now then myself with eaction and Thomas with extended matches (I
> >dont think the order of the last two matters). I will just code against
> >whatever latest release is and migrate later when all the otehr patches
> >are in.
> 
> Sounds good too me.

Fine with me. Jamal, we might want to share some code to do the logic
relations like sharing the macros to access the bits, checking if the
the current result already makes the whole expression obvious etc.

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

[jamal]

On Fri, 2004-12-31 at 06:26, Thomas Graf wrote:

>  Jamal, we might want to share some code to do the logic
> relations like sharing the macros to access the bits, checking if the
> the current result already makes the whole expression obvious etc.

Sure we can discuss - you may actually end up doing yours first and i
will use the LinuxWay(tm) ;-> aka inherit all your bugs ;->
I started working on it but too distracted finding some exciting stuff
on this e1000.

cheers,
jamal

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

[Arnaldo Carvalho de Melo]

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.

Agreed, whenever I have the chance, I remove such bloat ;)

- Arnaldo