lkml.org 
[lkml]   [2013]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: IPsec AH use of ahash


----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "David Dillow" <dave@thedillows.org>, "Borislav Petkov" <bp@alien8.de>, linux-kernel@vger.kernel.org,
> netdev@vger.kernel.org
> Sent: Monday, 21 January, 2013 9:37:41 AM
> Subject: Re: IPsec AH use of ahash
> >
> > I find that 73% of all stats are made up.
>
> I was only talking about my own experience. I gave no numbers.

That was a joke. You assumed that because I don't trim whitespace from multi-line comments [among other asinine code style issues] that I'm a bad developer. Yet what actually happened was a build configuration error in which the file wasn't being compiled fully.

Let it go.

> I was just giving examples of what I use. As those usually apply to
> what
> I do. If your code is affected by any configs, you should compile
> with
> them on and off to make sure you didn't break them. This is a bit
> more
> extensive testing, and not always required. But it does help to do
> so,
> as it becomes embarrassing if your code breaks on a config you didn't
> test.
>
> That's coming from my own experience too ;-)

Yup, I missed the self-test flag in the menu. That's full on my bad.

That said, it should be the opposite [default on] since self-testing should be relatively cheap and easy. Generally unless it's prohibitive you want as much self-test code-path testing in the default build as possible. Users who are tight on memory can turn it off if it suits their platform.

> > I actually did resubmit this morning with most of the checkpatch
> > issues fixed.
>
> Thank you.

Like I said I'm not trying to force everyone else to adopt to how we do things. I was just airing out a complaint from the point of view of a new submitter.

I still think checkpatch rules are full of sh!t but I know now to run code I submit through it regardless of where the code came from. :-)

> Your boss micro manages your time that much? And 45 mintes to do
> that?

Strictly speaking I haven't actually tried the code out in the lab yet. I was hoping that testmgr would run but it hasn't.

Realistically speaking none of the changes I made this morning should have any bearing on the correctness of the code. I'd be surprised if it failed in the lab.

> > Seriously, no spaces on the trailing edge of multi line comments?
> > :-/
>
> Some of checkpatch'es complaints are annoying. I'll grant you that.
> And
> checkpatch is more of a guideline than a strict rule. It's up to the
> maintainer of the code to determine how much checkpatch should be
> enforced.

That's not the impression I got from this weekends exchange.

> For example, checkpatch complains on code like:
>
> + asm volatile (
> +#ifdef CONFIG_X86_64
> + " xchg %%rbx,%%rsp \n"
> +#else
> + " xchgl %%ebx,%%esp \n"
> +#endif
> + " int3 \n"
> + " .globl jprobe_return_end\n"
> + " jprobe_return_end: \n"
> + " nop \n"::"b"
> + (kcb->jprobe_saved_sp):"memory");
>
> Because the white space before the '\n' is not needed. But adding
> that
> whitespace makes it easier to read the assembly.

So who gets to decide when/where to deviate from the rules?

> When enforcing checkpatch makes the code less readable, that's when
> it
> should be ignored. But again, that's really up to the maintainer of
> the
> code to decide.

So we divine what the maintainer wants and doesn't want when we submit patches? I think for me I'm going to follow them literally for now to avoid issues.

> > Anyways, I did re-submit. I still have no idea how testmgr works
> > but hopefully someone can pick it up from there.
>
> Well thank you again. This is the way the kernel community works.
> Just
> state you're not familiar with testmgr, and someone who is should
> check
> it out.

Hopefully :-)

Tom


\
 
 \ /
  Last update: 2013-01-21 16:43    [W:0.073 / U:1.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site