: tc action cleanup + fixes

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

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

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

(quoted diff) +1 -2

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

(quoted diff) +0 -2

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

jamal wrote:
Patrick,

Quoted 17 lines

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;

Quoted 3 lines

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.

Review

Quoted 6 lines

b)Other things which i have seen compilers whine about in the past of the form:

a missing cast

(quoted diff) +1 -1

-       a->priv = (void *) p;
+       a->priv = p;

No need to cast a pointer to void *, except if a->priv was of some different type.

Quoted 5 lines

4) Some of those messages are actually still useful and dont really harm to leave around for a little while longer;

(quoted diff) +0 -2

-       if (tb[TCA_PEDIT_PARMS - 1] == NULL) {
-               printk("BUG: tcf_pedit_init called with NULL params\n");

Quoted 4 lines

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.

Remove checks for impossible conditions in pedit action.

Quoted 3 lines

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.

Quoted 10 lines

Review

Quoted 6 lines

b)Other things which i have seen compilers whine about in the past of the form:

a missing cast

(quoted diff) +1 -1

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

Quoted 5 lines

Quoted 5 lines

4) Some of those messages are actually still useful and dont really harm to leave around for a little while longer;

(quoted diff) +0 -2

-       if (tb[TCA_PEDIT_PARMS - 1] == NULL) {
-               printk("BUG: tcf_pedit_init called with NULL params\n");

Quoted 9 lines

Quoted 4 lines

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:

Quoted 1 line

Remove checks for impossible conditions in pedit action.

________________________________________________________________________
[..]

(quoted diff) +0 -1

-	if (p == NULL) {

Review

Quoted 4 lines

(quoted diff) +0 -4

-		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 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, 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 :)

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.

Quoted 7 lines · jamal

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.

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.

Quoted 18 lines · 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;

Quoted 3 lines

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.