[Top] [All Lists]

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

To: hadi@xxxxxxxxxx
Subject: Re: [PATCH PKT_SCHED 0/17]: tc action cleanup + fixes
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Thu, 30 Dec 2004 13:34:52 +0100
Cc: Maillist netdev <netdev@xxxxxxxxxxx>
In-reply-to: <1104382562.1048.39.camel@xxxxxxxxxxxxxxxx>
References: <41D3785F.3040909@xxxxxxxxx> <1104382562.1048.39.camel@xxxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040413 Debian/1.6-5
jamal wrote:
Thanks for this cleanup.


1)compiler or style issue?

You have a few of fixes from

 if (..){
        single statement here;
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:

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


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