IPsec AH use of ahash

IPsec AH use of ahash

[Tom St Denis]

Hi all,

The AH4/6 code uses ahash to perform the MAC calculation but this precludes the availability of GMAC to the user.  Are there any plans to port the AH code over to the aead API so that it can use potentially any algo available to the user?

Tom St Denis

Re: IPsec AH use of ahash

[Steffen Klassert]

Please Cc netdev@vger.kernel.org on all networking related topics.

On Tue, Jan 15, 2013 at 11:51:48AM -0500, Tom St Denis wrote:
> Hi all,
> 
> The AH4/6 code uses ahash to perform the MAC calculation but this precludes the availability of GMAC to the user.  Are there any plans to port the AH code over to the aead API so that it can use potentially any algo available to the user?
> 

There was no need for this by now, but you are welcome
to send patches.

Re: IPsec AH use of ahash

[Tom St Denis]

Any "maintainers" going to reply to this at all?  

Tom

----- Original Message -----
> From: "Steffen Klassert" <steffen.klassert@secunet.com>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: herbert@gondor.apana.org.au, davem@davemloft.net, linux-kernel@vger.kernel.org, netdev@vger.kernel.org
> Sent: Wednesday, 16 January, 2013 1:21:04 AM
> Subject: Re: IPsec AH use of ahash
> 
> Please Cc netdev@vger.kernel.org on all networking related topics.
> 
> On Tue, Jan 15, 2013 at 11:51:48AM -0500, Tom St Denis wrote:
> > Hi all,
> > 
> > The AH4/6 code uses ahash to perform the MAC calculation but this
> > precludes the availability of GMAC to the user.  Are there any
> > plans to port the AH code over to the aead API so that it can use
> > potentially any algo available to the user?
> > 
> 
> There was no need for this by now, but you are welcome
> to send patches.
> 

Re: IPsec AH use of ahash

[David Miller]

From: Tom St Denis <tstdenis@elliptictech.com>
Date: Fri, 18 Jan 2013 14:35:33 -0500 (EST)

> Any "maintainers" going to reply to this at all?  

What do you mean?  There was a reply, and the reply if someone
so skilled finds this facility useful they can feel free to
submit an implementation.

You can't force people to work on something they have no interest
in, especially when it's effectively a new feature.

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "David Miller" <davem@davemloft.net>
> To: tstdenis@elliptictech.com
> Cc: "steffen klassert" <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au, linux-kernel@vger.kernel.org,
> netdev@vger.kernel.org
> Sent: Friday, 18 January, 2013 2:50:05 PM
> Subject: Re: IPsec AH use of ahash
> 
> From: Tom St Denis <tstdenis@elliptictech.com>
> Date: Fri, 18 Jan 2013 14:35:33 -0500 (EST)
> 
> > Any "maintainers" going to reply to this at all?
> 
> What do you mean?  There was a reply, and the reply if someone
> so skilled finds this facility useful they can feel free to
> submit an implementation.
> 
> You can't force people to work on something they have no interest
> in, especially when it's effectively a new feature.
> 

Admittedly I'm new to the kernel scene but what exactly is a "maintainer" then?

Suppose I invest time to re-write the IPv4/v6 AH code to correctly use AEAD instead of ahash, to then perform the testing required, etc... do I get credit as a maintainer in the IPsec tree?

I'm also a little annoyed that my CMAC patch was rejected for among other reasons that it violated "coding standards."  Specially since it was almost entirely copied from crypto/xcbc.c which also violates the same rules.  As a newcomer to the tree I tried my best by emulating readily available code (which apparently was already accepted into the tree) to then just get shot down for attempting to contribute.  If my CMAC code is not good enough for the tree I humbly suggest you also remove the XCBC code too while we're at it.

What I would expect from the "maintainers" is that they actually take on a more than trivial involvement in the progress of the code.  If I have to create original content and massage it into whatever form pleases the owners of the tree am I not the maintainer of the code?  I was honestly expecting someone with more involvement in the tree to move the [in this case] CMAC patch forward.  

As for the AH side if it's the implied intent of the maintainers to not support all relevant [and ideally low hanging fruit] standards as possible this should be stated explicitly.

Tom

Re: IPsec AH use of ahash

[Waskiewicz Jr, Peter P]

On Fri, Jan 18, 2013 at 03:53:44PM -0500, Tom St Denis wrote:
> Admittedly I'm new to the kernel scene but what exactly is a "maintainer" then?

Maintainers are ultimately responsible for content that flows into a tree
or subsystem.  That means they must command a level of expertise on
everything about the code they maintain to decide what goes in and what
doesn't.  This doesn't mean they know everything about everything, but
their judgement is built on experience and time in the job.

> Suppose I invest time to re-write the IPv4/v6 AH code to correctly use AEAD instead of ahash, to then perform the testing required, etc... do I get credit as a maintainer in the IPsec tree?

Working on a single thing doesn't make you a maintainer, it makes you a
contributor.  There are vastly more contributors than maintainers, and that's
how it should be.  A maintainer depends on contributors to be the people
in the trenches dealing with the specifics of things like the AH code, in
this case.  However, a maintainer's signed-off-by on a contributor's code
also makes the maintainer accountable for the code being included in the
kernel.  This is not something to be taken lightly.

> I'm also a little annoyed that my CMAC patch was rejected for among other reasons that it violated "coding standards."  Specially since it was almost entirely copied from crypto/xcbc.c which also violates the same rules.  As a newcomer to the tree I tried my best by emulating readily available code (which apparently was already accepted into the tree) to then just get shot down for attempting to contribute.  If my CMAC code is not good enough for the tree I humbly suggest you also remove the XCBC code too while we're at it.

I would recommend just hanging out on the netdev list and read it for a few
days.  There are a *large* number of patches that are critiqued and asked
to either be rewritten, modified, or dropped.  The whole point of the
community is to share your work and code, and then either defend the code,
or incorporate feedback and resubmit.  Many patches take multiple spins before
they're applied.  It's the nature of the community.

Note that quite a bit of code has been in the kernel for a long time.  There
will be examples of code that doesn't conform; there are millions of lines of
code in Linux, nothing is going to be perfect.  If you find your code doesn't
meet a coding standard, then respin the patch, and then use that opportunity
to fix the other code you've seen to meet the coding standard.  That's two
contributions that only improve the kernel, and increases your credibility
as an active contributor.

> What I would expect from the "maintainers" is that they actually take on a more than trivial involvement in the progress of the code.  If I have to create original content and massage it into whatever form pleases the owners of the tree am I not the maintainer of the code?  I was honestly expecting someone with more involvement in the tree to move the [in this case] CMAC patch forward.  

I'd suggest you go look at patchwork.ozlabs.org and look at the queues of
patches that various maintainers are juggling.  Maintainers offer feedback and
review when they can, but they rely on other contributors to really vet the
code.  And the fact that David responded at all shouldn't be viewed as
trivial involvement, he's one of the most busy maintainers in the kernel.

Cheers,
-PJ

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "David Miller" <davem@davemloft.net>, "steffen klassert" <steffen.klassert@secunet.com>,
> herbert@gondor.apana.org.au, linux-kernel@vger.kernel.org, netdev@vger.kernel.org
> Sent: Friday, 18 January, 2013 5:16:14 PM
> Subject: Re: IPsec AH use of ahash
> 
> On Fri, Jan 18, 2013 at 03:53:44PM -0500, Tom St Denis wrote:
> > Admittedly I'm new to the kernel scene but what exactly is a
> > "maintainer" then?
> 
> Maintainers are ultimately responsible for content that flows into a
> tree
> or subsystem.  That means they must command a level of expertise on
> everything about the code they maintain to decide what goes in and
> what
> doesn't.  This doesn't mean they know everything about everything,
> but
> their judgement is built on experience and time in the job.
> 
> > Suppose I invest time to re-write the IPv4/v6 AH code to correctly
> > use AEAD instead of ahash, to then perform the testing required,
> > etc... do I get credit as a maintainer in the IPsec tree?
> 
> Working on a single thing doesn't make you a maintainer, it makes you
> a
> contributor.  There are vastly more contributors than maintainers,
> and that's
> how it should be.  A maintainer depends on contributors to be the
> people
> in the trenches dealing with the specifics of things like the AH
> code, in
> this case.  However, a maintainer's signed-off-by on a contributor's
> code
> also makes the maintainer accountable for the code being included in
> the
> kernel.  This is not something to be taken lightly.

My gripe here is suppose I spend professional paid time working on an AH patch to [in my opinion] fix it and then I get ignored/stonewalled/etc because I didn't cross a t or dot an i ... what is my incentive to contribute publicly?  I can't go to my boss and get time to work on it if the likelihood of seeing it in mainline is basically around 0%.

The CMAC patch for instance was a dead simple patch that even modulo the missing () around an xor expression should have been a no-brainer to merge into their trees for the eventual 3.8 release cycle.  Instead I get nitpicked and frankly publicly bashed on coding quality despite the fact it's entirely based on existing UNMAINTAINED [apparently] kernel code.
 
> I would recommend just hanging out on the netdev list and read it for
> a few
> days.  There are a *large* number of patches that are critiqued and
> asked
> to either be rewritten, modified, or dropped.  The whole point of the
> community is to share your work and code, and then either defend the
> code,
> or incorporate feedback and resubmit.  Many patches take multiple
> spins before
> they're applied.  It's the nature of the community.

It's the nature of a hostile working environment.  Making me re-submit in the hopes of a) getting acknowledged and b) accepted is humiliating and frankly a waste of time.  

It would have taken them very little time to merge the patch I sent in, fix the (), maybe address some of the coding style "errors" and then form a patch for mainline based on that.  Don't you think I have better things to do than re-submit working code repeatedly?

> Note that quite a bit of code has been in the kernel for a long time.
>  There
> will be examples of code that doesn't conform; there are millions of
> lines of
> code in Linux, nothing is going to be perfect.  If you find your code
> doesn't
> meet a coding standard, then respin the patch, and then use that
> opportunity
> to fix the other code you've seen to meet the coding standard.
>  That's two
> contributions that only improve the kernel, and increases your
> credibility
> as an active contributor.

Ok but as "maintainers" they should be "maintaining" code ... if coding standards change (and btw the XCBC code was likely submitted long after the coding standards were put in place) then the maintainers should go through code they're responsible for and update it.

"xcbc.c" for instance was last touched in 2011.  It hasn't been maintained at all apparently.  There were a handful of patches against it ... none which address these "coding standards."
 
Tom

Re: IPsec AH use of ahash

[Michal Kubecek]

On Fri, Jan 18, 2013 at 05:31:45PM -0500, Tom St Denis wrote:
> My gripe here is suppose I spend professional paid time working on an
> AH patch to [in my opinion] fix it and then I get
> ignored/stonewalled/etc because I didn't cross a t or dot an i ...
...
> ... if the likelihood of seeing it in mainline is basically around 0%.

You received a detailed response from subsystem maintainer who is an
extremely busy man; that doesn't look like ignoring to me. And as for
stonewalling, I don't see anything like that either. You were just told
what is wrong and asked to send a fixed version. Once you do that, the
chances of the patch to be accepted are actually very high.

Yes, kernel rules for submitting and coding style may seem too strict at
the first glance. But there is a good reason for them: linux kernel is
huge and without strict rules it would be one big mess. Part of my work
consists of resolving bugs in various software projects, often these are
projects the sources of I have never seen before. And I wish more (or
rather as many as possible) had rules similar to the kernel because
looking for something in code which is badly structured and lacks
unified coding style, in project without good and descriptive commit
description, can be a terrible experience.

You may call it nitpicking and mock it, you may even feel offended, but
the open source world would be much better place to live in if other
projects had rules similar to linux kernel (and its networking in
particular).

> It would have taken them very little time to merge the patch I sent
> in, fix the (), maybe address some of the coding style "errors" and
> then form a patch for mainline based on that.  Don't you think I have
> better things to do than re-submit working code repeatedly?

So you consider your time too precious to conform to the kernel coding
style and, on the other hand, the time of subsystem maintainers totally
worthless so that you feel it is their job to tidy up your patches?

Someone already pointed you to http://patchwork.ozlabs.org/
Please do take a look there. I just did and found that in last three
months, about 3500 patches were submitted to this list, i.e. about
40 patches per day (including weekends and Christmas). All of these need
to be reviewed by a few maintainers who are also doing their part of
development. How do they manage to handle it, honestly I don't know.
And then you come and tell us they should also fix coding style and
obvious mistakes for everyone who is too lazy to do it themselves.
Don't you think it would be much more effective if we tried to make
their work easier rather than put more work on their shoulders?

> Ok but as "maintainers" they should be "maintaining" code ... if
> coding standards change (and btw the XCBC code was likely submitted
> long after the coding standards were put in place) then the
> maintainers should go through code they're responsible for and update
> it.

Fixing the wrong coding style of existing code would be definitely
useful but unlike reviewing patches, fixing bugs and developing
features, it doesn't require detailed knowledge of the code. Using
highly skilled and experienced developers (which is who subsystem
maintainers are), that would be a real wasting of resources.

> "xcbc.c" for instance was last touched in 2011.  It hasn't been
> maintained at all apparently.  There were a handful of patches against
> it ... none which address these "coding standards."

The fact that there are only few changes doesn't necessarily mean the
code is unmaintained. It can also mean that it works well and it doesn't
need new features or adjusting to new hardware or protocol versions. And
when the code doesn't change too often, the urge to fix its style is
rather low. But presence of old badly styled code doesn't justify
introducing more badly styled code.

                                                          Michal Kubecek

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Michal Kubecek" <mkubecek@suse.cz>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>, "David Miller" <davem@davemloft.net>, "steffen
> klassert" <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au, linux-kernel@vger.kernel.org,
> netdev@vger.kernel.org
> Sent: Friday, 18 January, 2013 9:33:55 PM
> Subject: Re: IPsec AH use of ahash
> 
> On Fri, Jan 18, 2013 at 05:31:45PM -0500, Tom St Denis wrote:
> > My gripe here is suppose I spend professional paid time working on
> > an
> > AH patch to [in my opinion] fix it and then I get
> > ignored/stonewalled/etc because I didn't cross a t or dot an i ...
> ...
> > ... if the likelihood of seeing it in mainline is basically around
> > 0%.
> 
> You received a detailed response from subsystem maintainer who is an
> extremely busy man; that doesn't look like ignoring to me. And as for
> stonewalling, I don't see anything like that either. You were just
> told
> what is wrong and asked to send a fixed version. Once you do that,
> the
> chances of the patch to be accepted are actually very high.

My objection is mostly that the feedback wasn't practical.  [more on that later].

> Yes, kernel rules for submitting and coding style may seem too strict
> at
> the first glance. But there is a good reason for them: linux kernel
> is
> huge and without strict rules it would be one big mess. Part of my
> work
> consists of resolving bugs in various software projects, often these
> are
> projects the sources of I have never seen before. And I wish more (or
> rather as many as possible) had rules similar to the kernel because
> looking for something in code which is badly structured and lacks
> unified coding style, in project without good and descriptive commit
> description, can be a terrible experience.

Having worked with a fair bit of Kernel code I can definitely say there isn't really a unified standard.  About the only thing Kernel developers agree on is they use C and don't comment their code.

> You may call it nitpicking and mock it, you may even feel offended,
> but
> the open source world would be much better place to live in if other
> projects had rules similar to linux kernel (and its networking in
> particular).

Having run several [admittedly much smaller] OSS projects I never turned away contributors with such raw efficiency as what I've seen here.

Sometimes that means you do the bitch work to format things pretty or document API changes or do any number of non-glorious tasks.  I consider it a professional crime that "use the source luke" is not merely a joke but an actual de facto work standard.  
 
> So you consider your time too precious to conform to the kernel
> coding
> style and, on the other hand, the time of subsystem maintainers
> totally
> worthless so that you feel it is their job to tidy up your patches?

Yes.  They're maintainers of code that millions of people use.  If they're too busy to take it on they should take their names off the maintainers list.

You don't get the credit without doing the work.  

> Someone already pointed you to http://patchwork.ozlabs.org/
> Please do take a look there. I just did and found that in last three
> months, about 3500 patches were submitted to this list, i.e. about
> 40 patches per day (including weekends and Christmas). All of these
> need
> to be reviewed by a few maintainers who are also doing their part of
> development. How do they manage to handle it, honestly I don't know.
> And then you come and tell us they should also fix coding style and
> obvious mistakes for everyone who is too lazy to do it themselves.
> Don't you think it would be much more effective if we tried to make
> their work easier rather than put more work on their shoulders?

Maybe the netdev project is too large then?  If they can't maintain it either there isn't enough staff or it's just too large to organize.

The RFC for AH-GMAC is hardly new yet the IPsec AH driver cannot support it.  Furthermore, AEAD is a better match for people [like me] doing hardware acceleration since it means I only have to write one set of drivers.  For instance I do null-cipher ESP through an AEAD driver that implements "ecb(null_cipher),foo" to cut down the code I have to write considerably.  If AH used AEAD I could fully remove my ahash code and have much less to worry about.

So from a user point of view AH is implemented poorly.
 
> Fixing the wrong coding style of existing code would be definitely
> useful but unlike reviewing patches, fixing bugs and developing
> features, it doesn't require detailed knowledge of the code. Using
> highly skilled and experienced developers (which is who subsystem
> maintainers are), that would be a real wasting of resources.

The problem is coding styles are those of the owners.  I don't professionally develop code anything like the Kernel format and I will *never* adopt it.

For instance

if (foo)
   statement;

Is horrible to me.  I've personally seen co-workers cause problems with that format [without the braces].  etc...

So realistically it's not fair to require contributors from all walks of life to adopt some random coding style that isn't even applied to the accepted code.

> > "xcbc.c" for instance was last touched in 2011.  It hasn't been
> > maintained at all apparently.  There were a handful of patches
> > against
> > it ... none which address these "coding standards."
> 
> The fact that there are only few changes doesn't necessarily mean the
> code is unmaintained. It can also mean that it works well and it
> doesn't
> need new features or adjusting to new hardware or protocol versions.
> And
> when the code doesn't change too often, the urge to fix its style is
> rather low. But presence of old badly styled code doesn't justify
> introducing more badly styled code.

That's ironic that you accept "if it works leave it alone" but at the same time maintain that standards exist for a reason and should be adhered to.

More so, I stand by what I said.  As a new contributor if I can't base my original content on existing content as a template for "proper design" then what can I?  How is this not simply "do as I say not as I do?"

Tom

Re: IPsec AH use of ahash

[Eric Dumazet]

On Sat, 2013-01-19 at 03:33 +0100, Michal Kubecek wrote:

> Someone already pointed you to http://patchwork.ozlabs.org/
> Please do take a look there. I just did and found that in last three
> months, about 3500 patches were submitted to this list, i.e. about
> 40 patches per day (including weekends and Christmas). All of these need
> to be reviewed by a few maintainers who are also doing their part of
> development. How do they manage to handle it, honestly I don't know.

I want to make here a huge thanks to David Miller, and of course
to all contributors.

I truly believe we did very well, and I really hope new contributors
will come and continue the impressive work.

Sure, sometime we can react not as good as we could do if we were
not overloaded. (I mean, 6 hours listening Lance Amstrong confession.
You really cant avoid such a scoop !)

I understand that for a new contributor, it might be difficult to
catch up with various rules, but reading netdev archives should be
enough to understand how it really works. Its not like the process
was a secret.

Tom, even a maintainer can make errors, thats not a big deal, as long
as things can go forward.

If you felt you had 0% chance to get your patch being accepted, then
you had a wrong feeling.

If the patch makes sense and you agree to address reviewers feedback, it
definitely can be accepted. If you think you don't have time to do that,
then maybe the patch is not really needed.

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Eric Dumazet" <erdnetdev@gmail.com>
> To: "Michal Kubecek" <mkubecek@suse.cz>
> Cc: "Tom St Denis" <tstdenis@elliptictech.com>, "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>, "David
> Miller" <davem@davemloft.net>, "steffen klassert" <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au,
> linux-kernel@vger.kernel.org, netdev@vger.kernel.org
> Sent: Friday, 18 January, 2013 10:59:55 PM
> Subject: Re: IPsec AH use of ahash
> 
> On Sat, 2013-01-19 at 03:33 +0100, Michal Kubecek wrote:
> 
> > Someone already pointed you to http://patchwork.ozlabs.org/
> > Please do take a look there. I just did and found that in last
> > three
> > months, about 3500 patches were submitted to this list, i.e. about
> > 40 patches per day (including weekends and Christmas). All of these
> > need
> > to be reviewed by a few maintainers who are also doing their part
> > of
> > development. How do they manage to handle it, honestly I don't
> > know.
> 
> I want to make here a huge thanks to David Miller, and of course
> to all contributors.
> 
> I truly believe we did very well, and I really hope new contributors
> will come and continue the impressive work.
> 
> Sure, sometime we can react not as good as we could do if we were
> not overloaded. (I mean, 6 hours listening Lance Amstrong confession.
> You really cant avoid such a scoop !)

As someone who maintained (and I mean that in all senses not just applied patches) OSS projects while working full time and still trying to have a life I get it.  That said I never turned away patches solely on "style" issues.  At the same time you should look at the size and scope of the patches I'm talking about.  It took me all of less than an hour to develop and less than a couple of hours to give it a run to see if it works correctly [talking about the CMAC patch].

This *should* be a no-brainer to apply.

> I understand that for a new contributor, it might be difficult to
> catch up with various rules, but reading netdev archives should be
> enough to understand how it really works. Its not like the process
> was a secret.
> 
> Tom, even a maintainer can make errors, thats not a big deal, as long
> as things can go forward.
> 
> If you felt you had 0% chance to get your patch being accepted, then
> you had a wrong feeling.

My problem is the lack of ownership of the task from the maintainers.  For instance, I was surprised that CryptoAPI didn't support CMAC already given that it's a NIST standard (more than XCBC is).  The maintainers of CryptoAPI deemed fit to add all sorts of non-standard nonsense like Serpent and Twofish and heck even VMAC but not CMAC?  Who's goals are they serving here?

Then I get time from my employer to add CMAC to the kernel so I base it off the closest match, XCBC.  I modify one function, add it's name to a table and fire off a patch.  What happens?  It gets ignored.  Then I resubmit it with he maintainers in the cc: and I get an ack [this is during 3.7-rc...].  The 3.8 window opens up and .... the code is nowhere to be found.  I ask again, and then I get "it doesn't match our coding standards, please try again."

> If the patch makes sense and you agree to address reviewers feedback,
> it
> definitely can be accepted. If you think you don't have time to do
> that,
> then maybe the patch is not really needed.

The typical OSS cop-out.  It's not about what *you* deem important.  It's what the customer/users do.  And in this case I went the extra yard and submitted working code.  I shouldn't have to resubmit working code because of "style" issues.  The maintainers should be damn grateful that I submitted [working] code at all and they can spend the time to integrate it into mainline.

The fact is right now *I* have the ability to perform CMAC in the kernel.  Nobody else does.  So aside from annoying some of my users by having them patch their kernel with a patch I provide them we're not missing out on functionality.  It's the rest of the Kernel users (whom I don't interact with) who are now missing functionality.

This gets back to the AH AEAD case.  Suppose I take the existing ah4.c and just re-write the ahash statements to use aead and touch nothing else... if the existing ah4.c doesn't meet coding standards will that patch get tossed out too?  Do you see perhaps how impractical that standard is?

For those of us who do Kernel development during business hours it's hard to justify the work when the path to mainline is convoluted and landmined.  

Tom

Re: IPsec AH use of ahash

[Eric Dumazet]

On Sat, 2013-01-19 at 05:30 -0500, Tom St Denis wrote:

> As someone who maintained (and I mean that in all senses not just
> applied patches) OSS projects while working full time and still trying
> to have a life I get it.  That said I never turned away patches solely
> on "style" issues.  At the same time you should look at the size and
> scope of the patches I'm talking about.  It took me all of less than
> an hour to develop and less than a couple of hours to give it a run to
> see if it works correctly [talking about the CMAC patch].
> 
> This *should* be a no-brainer to apply.
> 

Again, if its a no-brainer, the patch should be perfect, so that our
brains can cope with the hard to review patches, and hard to debug
problems. A maintainer is not a machine doing mechanical work

Every time we have to ask you another round, its a lost of time for
everybody.

Really if you can not understand that, thats too bad.


> For those of us who do Kernel development during business hours it's
> hard to justify the work when the path to mainline is convoluted and
> landmined.  

Thousands of contributors just did it very well. 

You might read a bit Documentation/SubmittingPatches

vi +263 Documentation/SubmittingPatches

This is a bit outdated, as obviously Linus is not the guy most
contributors are dealing with.

Re: IPsec AH use of ahash

[Mike Galbraith]

On Sat, 2013-01-19 at 05:30 -0500, Tom St Denis wrote:

> For those of us who do Kernel development during business hours it's
> hard to justify the work when the path to mainline is convoluted and
> landmined.

Sounds as though any patches you submit land on your dinner plate just
like potatoes.  Hand the cook a pot of half peeled potatoes, he/she may
say try again.  The result of a little extra effort is tastier taters
for everybody feasting at the common table.. including you.

-Mike

Re: IPsec AH use of ahash

[Borislav Petkov]

On Sun, Jan 20, 2013 at 06:06:21AM +0100, Mike Galbraith wrote:
> Sounds as though any patches you submit land on your dinner plate just
> like potatoes. Hand the cook a pot of half peeled potatoes, he/she may
> say try again.

And how he/she would say it! I've seen serious fights spring up from
half-peeled potatoes so people better peel them right the first time.

:-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Borislav Petkov" <bp@alien8.de>
> To: "Mike Galbraith" <bitbucket@online.de>
> Cc: "Tom St Denis" <tstdenis@elliptictech.com>, "Eric Dumazet" <erdnetdev@gmail.com>, "Waskiewicz Jr, Peter P"
> <peter.p.waskiewicz.jr@intel.com>, "David Miller" <davem@davemloft.net>, "steffen klassert"
> <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
> "Michal Kubecek" <mkubecek@suse.cz>
> Sent: Sunday, 20 January, 2013 5:31:32 AM
> Subject: Re: IPsec AH use of ahash
> 
> On Sun, Jan 20, 2013 at 06:06:21AM +0100, Mike Galbraith wrote:
> > Sounds as though any patches you submit land on your dinner plate
> > just
> > like potatoes. Hand the cook a pot of half peeled potatoes, he/she
> > may
> > say try again.
> 
> And how he/she would say it! I've seen serious fights spring up from
> half-peeled potatoes so people better peel them right the first time.

You should really try running checkpatch.pl over code that's already in the kernel before you call out new contributors on it.

How is this supposed to not be adversarial when I can't even use the Kernel source itself as a reference?

Tom

Re: IPsec AH use of ahash

[Alexander Holler]

Am 20.01.2013 13:56, schrieb Tom St Denis:

> You should really try running checkpatch.pl over code that's already in the kernel before you call out new contributors on it.
>
> How is this supposed to not be adversarial when I can't even use the Kernel source itself as a reference?

In case of the kernel the chicken and egg problem can be answered 
without any questions, most source existed before checkpatch.pl (evolved 
to the current state).

Regards,

Alexander

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Alexander Holler" <holler@ahsoftware.de>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Borislav Petkov" <bp@alien8.de>, "Eric Dumazet" <erdnetdev@gmail.com>, "Waskiewicz Jr, Peter P"
> <peter.p.waskiewicz.jr@intel.com>, "David Miller" <davem@davemloft.net>, "steffen klassert"
> <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
> "Michal Kubecek" <mkubecek@suse.cz>, "Mike Galbraith" <bitbucket@online.de>
> Sent: Sunday, 20 January, 2013 8:34:20 AM
> Subject: Re: IPsec AH use of ahash
> 
> Am 20.01.2013 13:56, schrieb Tom St Denis:
> 
> > You should really try running checkpatch.pl over code that's
> > already in the kernel before you call out new contributors on it.
> >
> > How is this supposed to not be adversarial when I can't even use
> > the Kernel source itself as a reference?
> 
> In case of the kernel the chicken and egg problem can be answered
> without any questions, most source existed before checkpatch.pl
> (evolved
> to the current state).

We clearly have different interpretations of the word "maintainer" then...  If they're not maintaining the code then are they really the maintainers of it?

Point is I copied accepted kernel code and was rejected because of "errors" that are in existing kernel code.  Similarly if I did the upgrade to AH to use AEAD I suspect it would be rejected for the same reason.

Tom

Re: IPsec AH use of ahash

[Jan Engelhardt]

On Sunday 2013-01-20 14:54, Tom St Denis wrote:
>> 
>> > You should really try running checkpatch.pl over code that's
>> > already in the kernel before you call out new contributors on it.
>> >
>> > How is this supposed to not be adversarial when I can't even use
>> > the Kernel source itself as a reference?
>> 
>> In case of the kernel the chicken and egg problem can be answered
>> without any questions, most source existed before checkpatch.pl
>> (evolved
>> to the current state).
>
>We clearly have different interpretations of the word "maintainer"
>then... If they're not maintaining the code then are they really the
>maintainers of it?

Maintainers maintain code. From own experience - I too have some
projects that are *only* "maintained" - that includes maintaining the
status quo, keeping it compiling and taking patches that require no
more than 5 minutes to think about.

To support users, especially ones with fire-and-forget submissions,
an "integrator" (for the lack of a better word for cleaning guy)
or "developer" to take up their posting is required.
But it often lacks one or more.

Re: IPsec AH use of ahash

[Steven Rostedt]

On Sun, Jan 20, 2013 at 07:56:27AM -0500, Tom St Denis wrote:
> 
> You should really try running checkpatch.pl over code that's already in the kernel before you call out new contributors on it.
> 
> How is this supposed to not be adversarial when I can't even use the Kernel source itself as a reference?

So there's a lot of crap code in the kernel (that we are trying to clean
up when we have the time... see Kernel Janitors). But that's still no
excuse to allow more crap code to enter. That means adding more crap to
clean up.

-- Steve

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Borislav Petkov" <bp@alien8.de>, "Eric Dumazet" <erdnetdev@gmail.com>, "Waskiewicz Jr, Peter P"
> <peter.p.waskiewicz.jr@intel.com>, "David Miller" <davem@davemloft.net>, "steffen klassert"
> <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
> "Michal Kubecek" <mkubecek@suse.cz>, "Mike Galbraith" <bitbucket@online.de>
> Sent: Sunday, 20 January, 2013 5:07:22 PM
> Subject: Re: IPsec AH use of ahash
> 
> On Sun, Jan 20, 2013 at 07:56:27AM -0500, Tom St Denis wrote:
> > 
> > You should really try running checkpatch.pl over code that's
> > already in the kernel before you call out new contributors on it.
> > 
> > How is this supposed to not be adversarial when I can't even use
> > the Kernel source itself as a reference?
> 
> So there's a lot of crap code in the kernel (that we are trying to
> clean
> up when we have the time... see Kernel Janitors). But that's still no
> excuse to allow more crap code to enter. That means adding more crap
> to
> clean up.

Nowhere in the coding guidelines I've seen thus far says that you have to comment or document your code.

I wouldn't consider fixing "crap" code to mean to change indentation style [unless the original is horrible].  It's pointless busy work at best.

If you want to improve the quality of the code I'd start with /* and end with */ once in a while.

Tom

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Mike Galbraith" <bitbucket@online.de>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Eric Dumazet" <erdnetdev@gmail.com>, "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>, "David Miller"
> <davem@davemloft.net>, "steffen klassert" <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au,
> linux-kernel@vger.kernel.org, netdev@vger.kernel.org, "Michal Kubecek" <mkubecek@suse.cz>
> Sent: Sunday, 20 January, 2013 12:06:21 AM
> Subject: Re: IPsec AH use of ahash
> 
> On Sat, 2013-01-19 at 05:30 -0500, Tom St Denis wrote:
> 
> > For those of us who do Kernel development during business hours
> > it's
> > hard to justify the work when the path to mainline is convoluted
> > and
> > landmined.
> 
> Sounds as though any patches you submit land on your dinner plate
> just
> like potatoes.  Hand the cook a pot of half peeled potatoes, he/she
> may
> say try again.  The result of a little extra effort is tastier taters
> for everybody feasting at the common table.. including you.

No, in reality what happened is the chef made potatos [incorrectly] got busy and asked others to help out and make more potatos.  Then came back and said ...

from ah4.c (which I copied into foo4.c to prove a point) ... that is currently in the kernel

WARNING: networking block comments put the trailing */ on a separate line
#94: FILE: net/ipv4/foo4.c:76:
+ * for validity, so paranoia is not required. */

ERROR: spaces required around that '<' (ctx:VxV)
#112: FILE: net/ipv4/foo4.c:94:
+		if (optlen<2 || optlen>l)
 		          ^

ERROR: spaces required around that '>' (ctx:VxV)
#112: FILE: net/ipv4/foo4.c:94:
+		if (optlen<2 || optlen>l)
 		                      ^

ERROR: do not use assignment in if condition
#180: FILE: net/ipv4/foo4.c:162:
+	if ((err = skb_cow_data(skb, 0, &trailer)) < 0)

WARNING: line over 80 characters
#223: FILE: net/ipv4/foo4.c:205:
+		ah->hdrlen  = (XFRM_ALIGN4(sizeof(*ah) + ahp->icv_trunc_len) >> 2) - 2;

WARNING: line over 80 characters
#225: FILE: net/ipv4/foo4.c:207:
+		ah->hdrlen  = (XFRM_ALIGN8(sizeof(*ah) + ahp->icv_trunc_len) >> 2) - 2;

ERROR: spaces required around that ':' (ctx:VxW)
#281: FILE: net/ipv4/foo4.c:263:
+	err = memcmp(icv, auth_data, ahp->icv_trunc_len) ? -EBADMSG: 0;
 	                                                           ^

WARNING: networking block comments put the trailing */ on a separate line
#337: FILE: net/ipv4/foo4.c:319:
+	 * so... Later this can change. */

ERROR: do not use assignment in if condition
#345: FILE: net/ipv4/foo4.c:327:
+	if ((err = skb_cow_data(skb, 0, &trailer)) < 0)

ERROR: spaces required around that ':' (ctx:VxW)
#395: FILE: net/ipv4/foo4.c:377:
+	err = memcmp(icv, auth_data, ahp->icv_trunc_len) ? -EBADMSG: 0;
 	                                                           ^

WARNING: space prohibited between function name and open parenthesis '('
#407: FILE: net/ipv4/foo4.c:389:
+	kfree (work_iph);

WARNING: line over 80 characters
#416: FILE: net/ipv4/foo4.c:398:
+	struct ip_auth_hdr *ah = (struct ip_auth_hdr *)(skb->data+(iph->ihl<<2));

WARNING: line over 80 characters
#429: FILE: net/ipv4/foo4.c:411:
+	x = xfrm_state_lookup(net, skb->mark, (const xfrm_address_t *)&iph->daddr,

WARNING: networking block comments don't use an empty /* line, use /* Comment...
#466: FILE: net/ipv4/foo4.c:448:
+
+	/*

ERROR: that open brace { should be on the previous line
#522: FILE: net/ipv4/foo4.c:504:
+static const struct xfrm_type ah_type =
+{

WARNING: please, no space before tabs
#525: FILE: net/ipv4/foo4.c:507:
+^I.proto^I     ^I= IPPROTO_AH,$

total: 7 errors, 9 warnings, 547 lines checked


So going back to my original point ... Had I upgraded AH4/AH6 to use AEAD you guys would have rejected it because of style issues too?

The maintainers are not MAINTAINING the code.  Then they call out sous-chef's that come by offering to contribute because the recipe is not being followed...

Tom

Re: IPsec AH use of ahash

[Mike Galbraith]

On Sun, 2013-01-20 at 07:55 -0500, Tom St Denis wrote: 
> 
> ----- Original Message -----
> > From: "Mike Galbraith" <bitbucket@online.de>
> > To: "Tom St Denis" <tstdenis@elliptictech.com>
> > Cc: "Eric Dumazet" <erdnetdev@gmail.com>, "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>, "David Miller"
> > <davem@davemloft.net>, "steffen klassert" <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au,
> > linux-kernel@vger.kernel.org, netdev@vger.kernel.org, "Michal Kubecek" <mkubecek@suse.cz>
> > Sent: Sunday, 20 January, 2013 12:06:21 AM
> > Subject: Re: IPsec AH use of ahash
> > 
> > On Sat, 2013-01-19 at 05:30 -0500, Tom St Denis wrote:
> > 
> > > For those of us who do Kernel development during business hours
> > > it's
> > > hard to justify the work when the path to mainline is convoluted
> > > and
> > > landmined.
> > 
> > Sounds as though any patches you submit land on your dinner plate
> > just
> > like potatoes.  Hand the cook a pot of half peeled potatoes, he/she
> > may
> > say try again.  The result of a little extra effort is tastier taters
> > for everybody feasting at the common table.. including you.
> 
> No, in reality what happened is the chef made potatos [incorrectly]
> got busy and asked others to help out and make more potatos.  Then
> came back and said ...

Bottom line: either you grit your teeth and try again or you don't.
Calling the chef a big meanie doesn't put taters on dinner plates.

-Mike

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Mike Galbraith" <bitbucket@online.de>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Eric Dumazet" <erdnetdev@gmail.com>, "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>, "David Miller"
> <davem@davemloft.net>, "steffen klassert" <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au,
> linux-kernel@vger.kernel.org, netdev@vger.kernel.org, "Michal Kubecek" <mkubecek@suse.cz>
> Sent: Sunday, 20 January, 2013 9:11:34 AM
> Subject: Re: IPsec AH use of ahash
> 
> On Sun, 2013-01-20 at 07:55 -0500, Tom St Denis wrote:
> > 
> > ----- Original Message -----
> > > From: "Mike Galbraith" <bitbucket@online.de>
> > > To: "Tom St Denis" <tstdenis@elliptictech.com>
> > > Cc: "Eric Dumazet" <erdnetdev@gmail.com>, "Waskiewicz Jr, Peter
> > > P" <peter.p.waskiewicz.jr@intel.com>, "David Miller"
> > > <davem@davemloft.net>, "steffen klassert"
> > > <steffen.klassert@secunet.com>, herbert@gondor.apana.org.au,
> > > linux-kernel@vger.kernel.org, netdev@vger.kernel.org, "Michal
> > > Kubecek" <mkubecek@suse.cz>
> > > Sent: Sunday, 20 January, 2013 12:06:21 AM
> > > Subject: Re: IPsec AH use of ahash
> > > 
> > > On Sat, 2013-01-19 at 05:30 -0500, Tom St Denis wrote:
> > > 
> > > > For those of us who do Kernel development during business hours
> > > > it's
> > > > hard to justify the work when the path to mainline is
> > > > convoluted
> > > > and
> > > > landmined.
> > > 
> > > Sounds as though any patches you submit land on your dinner plate
> > > just
> > > like potatoes.  Hand the cook a pot of half peeled potatoes,
> > > he/she
> > > may
> > > say try again.  The result of a little extra effort is tastier
> > > taters
> > > for everybody feasting at the common table.. including you.
> > 
> > No, in reality what happened is the chef made potatos [incorrectly]
> > got busy and asked others to help out and make more potatos.  Then
> > came back and said ...
> 
> Bottom line: either you grit your teeth and try again or you don't.
> Calling the chef a big meanie doesn't put taters on dinner plates.

One point you're missing is that *I* have CMAC support.  *YOU* don't.  So being contrary and adversarial about it isn't really hurting me, it's annoying me because I have to manually supply a patch for my users but at the end of the day it isn't me who is losing out.  It seems odd that the maintainers who should be happy to receive original content which adds standards support are so ardent that it must be done perfectly and totally unlike the code they submit (which I've already shown doesn't meet these standards).

The other point is that the system can use some working on and unless people raise concerns nothing will change.  Large swathes of kernel code don't meet these "coding standards" despite the fact that many source files that are in violation have been "worked on" long after the checkpatch script was written.

In all likelihood I will submit a revised CMAC patch but it'll take time before I can get business hours to work on it.  So instead of having a maintainer just touch it up we're all going to lose out because of pride?

Tom

Re: IPsec AH use of ahash

[David Dillow]

On Sun, 2013-01-20 at 10:07 -0500, Tom St Denis wrote:
> In all likelihood I will submit a revised CMAC patch but it'll take
> time before I can get business hours to work on it.  So instead of
> having a maintainer just touch it up we're all going to lose out
> because of pride?

Yes -- but it would seem to be yours that presents the problem.

I'm sure you could have fixed your patch up in the amount of time you've
spent railing against the push-back. How much of that were you billing
for, and is that a productive use your employer's money? Even on your
own time, how productive has that been? Do you feel better, having
vented?

You don't think the maintainers are maintaining if they don't clean up
after every random contributor -- fine, call them lieutenants then.
Their job is to guide the new development towards the goals for the
system, review patches, and develop new features.

You'll notice I didn't say "reformat code when standards change." This
is a distributed project and it doesn't scale to have the code
continually in flux -- reformatting creates conflicts in other
contributors patches, which then consume all of a lieutenant's time if
they were to try to fix up each one for them. It's much better to push
push that work out to the edge of the network -- put another way, "many
hands make light work." Fixing existing formatting problems is
acceptable in a patch series if one is working in the area, but it is
rare that a series devoted solely to that kind of cleanup gets in.

The coding styles have evolved over time, and are different for
different areas of the kernel -- for example, most of the kernel wants
'/*' on its own line for a multi-line comment, but under net/ it should
not. You should try to match the style in your area -- claiming that the
cryto/ code does it one way is unlikely to sway opinion in net/.
Different lieutenants, different opinions.

As for the existing style issues in net/ipv4/ah4.c you posted, no, your
patch adding a feature in ah4 would probably not been rejected because
of the existing issues, though if one was in the middle of your changes,
it is possible that your would be asked to fix it. As for the issues
checkpatch.pl found, they are in many cases legitimate, but are also the
exceptions -- there are plenty of counter-examples in that file showing
the preferred style.

All the best for your future endeavors,
Dave

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "David Dillow" <dave@thedillows.org>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org
> Sent: Sunday, 20 January, 2013 11:34:52 AM
> Subject: Re: IPsec AH use of ahash
> 
> On Sun, 2013-01-20 at 10:07 -0500, Tom St Denis wrote:
> > In all likelihood I will submit a revised CMAC patch but it'll take
> > time before I can get business hours to work on it.  So instead of
> > having a maintainer just touch it up we're all going to lose out
> > because of pride?
> 
> Yes -- but it would seem to be yours that presents the problem.

Not really.  Again, *I* have this content.  You do not.  And the only reason you don't is because someone with pull-request authority is too lazy [or apathetic] to make minor cosmetic changes and get the change pushed through the process.
 
> I'm sure you could have fixed your patch up in the amount of time
> you've
> spent railing against the push-back. How much of that were you
> billing
> for, and is that a productive use your employer's money? Even on your
> own time, how productive has that been? Do you feel better, having
> vented?

I was hoping to have a constructive conversation in which the maintainers could come clean about the complete hypocrisy of claiming that the source is the reference [since there is no documentation for anything in the kernel] and then saying you can't actually read the source when you want to contribute. Then to tell me I have to adhere to some coding standard (which all things aside I don't agree with in the first place) despite the fact that *none* of the code in the kernel complies with it.

Furthermore I don't think you get how the business side of this works.

I was tasked with getting CMAC to work with IPsec.  I did.  I sent the patch off to the LKML.  My boss tasked me with other work (totally unrelated to IPsec) since in their eyes the project was done [we do after all have CMAC support].  

To then go back to my boss and say I need another couple of hours to "clean" it, re-test it in our IPsec lab and then resubmit it in hopes that the "maintainers" don't find some other reason to reject it is totally unprofessional and unproductive.

> You don't think the maintainers are maintaining if they don't clean
> up
> after every random contributor -- fine, call them lieutenants then.
> Their job is to guide the new development towards the goals for the
> system, review patches, and develop new features.

Know this.  Being a software developer involves doing a lot of lowly work that most people aren't excited by.  You guys already don't comment/document your code.  It's unreasonable to assume you can't at least maintain your own style guidelines... I mean what does a maintainer do?

> You'll notice I didn't say "reformat code when standards change."
> This
> is a distributed project and it doesn't scale to have the code
> continually in flux -- reformatting creates conflicts in other
> contributors patches, which then consume all of a lieutenant's time
> if
> they were to try to fix up each one for them. It's much better to
> push
> push that work out to the edge of the network -- put another way,
> "many
> hands make light work." Fixing existing formatting problems is
> acceptable in a patch series if one is working in the area, but it is
> rare that a series devoted solely to that kind of cleanup gets in.

Because the volunteers don't want to do lowly bitch work.

I'm not a volunteer.  I was paid to write the CMAC patch since our project supports it [amongst other modes including AH-GMAC...].   But that said we don't work on kernel time.  I don't bend my schedule around the whims of some moderator who is too damn lazy to do a bit of lowly work that they think is beneath them.
 
> The coding styles have evolved over time, and are different for
> different areas of the kernel -- for example, most of the kernel
> wants
> '/*' on its own line for a multi-line comment, but under net/ it
> should
> not. You should try to match the style in your area -- claiming that
> the
> cryto/ code does it one way is unlikely to sway opinion in net/.
> Different lieutenants, different opinions.

Which of course is asinine.
 
> As for the existing style issues in net/ipv4/ah4.c you posted, no,
> your
> patch adding a feature in ah4 would probably not been rejected
> because
> of the existing issues, though if one was in the middle of your
> changes,
> it is possible that your would be asked to fix it. As for the issues
> checkpatch.pl found, they are in many cases legitimate, but are also
> the
> exceptions -- there are plenty of counter-examples in that file
> showing
> the preferred style.

Except that my CMAC content came almost exclusively from XCBC which was already in the kernel.  Since you guys did reject the patch on the grounds of coding style why would I assume my AH patches would be any different?

Tom

Re: IPsec AH use of ahash

[David Dillow]

On Sun, 2013-01-20 at 12:40 -0500, Tom St Denis wrote:
> > On Sun, 2013-01-20 at 10:07 -0500, Tom St Denis wrote:
> > > In all likelihood I will submit a revised CMAC patch but it'll take
> > > time before I can get business hours to work on it.  So instead of
> > > having a maintainer just touch it up we're all going to lose out
> > > because of pride?
> > 
> > Yes -- but it would seem to be yours that presents the problem.
> 
> Not really.  Again, *I* have this content.  You do not. 

You mean the content at http://lkml.org/lkml/2012/12/11/369 ?

Funny how posting to an mailing list works...

>  And the only reason you don't is because someone with pull-request
> authority is too lazy [or apathetic] to make minor cosmetic changes
> and get the change pushed through the process.

Someone like the author?

> I was hoping to have a constructive conversation in which the
> maintainers could come clean about the complete hypocrisy of claiming
> that the source is the reference [since there is no documentation for
> anything in the kernel] and then saying you can't actually read the
> source when you want to contribute.

A conversation where you have already decided the outcome -- that's a
speech, not a dialogue.

> Furthermore I don't think you get how the business side of this works.

I've been doing both kernel and proprietary development for close to two
decades, so you might be surprised. I see it the other way around -- you
don't see how the open source part works, and are expecting to
substitute your proprietary process -- where crap code that "works" is
preferred, to hell with trying to maintain consistency, maintainability,
or even readability.

I've worked on those code bases, and the kernel is a breath of fresh air
in comparison.

> I was tasked with getting CMAC to work with IPsec.  I did.  I sent the
> patch off to the LKML.  My boss tasked me with other work (totally
> unrelated to IPsec) since in their eyes the project was done [we do
> after all have CMAC support].
> 
> To then go back to my boss and say I need another couple of hours to
> "clean" it, re-test it in our IPsec lab and then resubmit it in hopes
> that the "maintainers" don't find some other reason to reject it is
> totally unprofessional and unproductive.

You sold your boss a bill of goods, then -- that may fly in your shop,
but that doesn't work here. It is well known that there is likely to be
several rounds of review, and this isn't a new thing.

> > You don't think the maintainers are maintaining if they don't clean
> > up
> > after every random contributor -- fine, call them lieutenants then.
> > Their job is to guide the new development towards the goals for the
> > system, review patches, and develop new features.
> 
> Know this.  Being a software developer involves doing a lot of lowly
> work that most people aren't excited by.  You guys already don't
> comment/document your code.

I do know this, and I also know that the kernel is much better
documented than the most of the closed source code bases I've looked at
in my career.

> I'm not a volunteer.  I was paid to write the CMAC patch since our
> project supports it [amongst other modes including AH-GMAC...].   But
> that said we don't work on kernel time.  I don't bend my schedule
> around the whims of some moderator who is too damn lazy to do a bit of
> lowly work that they think is beneath them.

You think davem is lazy? Gruff, sure, but lazy? Well, thank you for
illuminating your grasp of reality.

> > As for the existing style issues in net/ipv4/ah4.c you posted, no,
> > your patch adding a feature in ah4 would probably not been rejected
> > because of the existing issues, though if one was in the middle of your
> > changes, it is possible that your would be asked to fix it.

> Except that my CMAC content came almost exclusively from XCBC which
> was already in the kernel.  Since you guys did reject the patch on the
> grounds of coding style why would I assume my AH patches would be any
> different?

There's a difference between patching an existing file, and introducing
a new one. Should you have introduced a new file based on AH, I suspect
it would also need some cleaning up.

Anyways, I'll stop feeding the troll now. You've made it quite clear
that no one is going to convince you, and calling the maintainers lazy
and hypocrites isn't going to convince them.

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "David Dillow" <dave@thedillows.org>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org
> Sent: Sunday, 20 January, 2013 1:11:11 PM
> Subject: Re: IPsec AH use of ahash
> 
> On Sun, 2013-01-20 at 12:40 -0500, Tom St Denis wrote:
> > > On Sun, 2013-01-20 at 10:07 -0500, Tom St Denis wrote:
> > > > In all likelihood I will submit a revised CMAC patch but it'll
> > > > take
> > > > time before I can get business hours to work on it.  So instead
> > > > of
> > > > having a maintainer just touch it up we're all going to lose
> > > > out
> > > > because of pride?
> > > 
> > > Yes -- but it would seem to be yours that presents the problem.
> > 
> > Not really.  Again, *I* have this content.  You do not.
> 
> You mean the content at http://lkml.org/lkml/2012/12/11/369 ?
> 
> Funny how posting to an mailing list works...

I posted that in December ... it wasn't till January that I got the first reply back about it failing to meet the coding "standards."  Do you honestly expect people to sit on their hands for a month each time simple requests float by?

Furthermore, it's not merged into mainline.  So no, you don't have the content.  

> >  And the only reason you don't is because someone with pull-request
> > authority is too lazy [or apathetic] to make minor cosmetic changes
> > and get the change pushed through the process.
> 
> Someone like the author?

You mean the maintainer right?  If I have to do all of the work I should get all of the credit.
  
> > Furthermore I don't think you get how the business side of this
> > works.
> 
> I've been doing both kernel and proprietary development for close to
> two
> decades, so you might be surprised. I see it the other way around --
> you
> don't see how the open source part works, and are expecting to
> substitute your proprietary process -- where crap code that "works"
> is
> preferred, to hell with trying to maintain consistency,
> maintainability,
> or even readability.

Um ok that's uncalled for.  What part of the fact my CMAC code is like 90% verbatim copied from the XCBC code do you not understand?   If you think the CMAC patch was bad then you better call up the authors of the XCBC code and call them out for it too.  Stop being hypocritical.

You claimed my code fails to maintain "consistency" ... IT'S COPIED FROM EXISTING CODE.  Your complaint makes no sense not to mention it's insulting.

Furthermore, as someone who ran OSS projects (some of which are in millions of devices around the world) I understood to cherish user contributions.  Even if they needed polishing up.  I did the bitch work of documenting, testing, packaging, development, debugging, and support.  It's far far far too common to see OSS developers shy away from the less fun side of software development...

I called out the maintainers here because because not only do they not maintain the integrity of the code for which they claim authority but they are hostile towards outside contributions with ridiculous standards that they themselves don't even adhere to.  Code that they work on in the kernel doesn't meet ANY of the standards set forth that I was supposed to magically divine applied here..
 
> You sold your boss a bill of goods, then -- that may fly in your
> shop,
> but that doesn't work here. It is well known that there is likely to
> be
> several rounds of review, and this isn't a new thing.

Ok sure but why does that involve me re-writing the patch for cosmetic reasons?  I agreed that adding the () around the ^/& expressions would be a good idea.  However, how is it a good use of my time to re-write the patch (which includes lines of code I didn't write in the first place) for coding styles that are not actually used in the kernel?

I **COPIED** existing code.  Yet somehow my code isn't good enough?

> I do know this, and I also know that the kernel is much better
> documented than the most of the closed source code bases I've looked
> at
> in my career.

That's not really an excuse for lack of documentation/comments.  

> You think davem is lazy? Gruff, sure, but lazy? Well, thank you for
> illuminating your grasp of reality.

I was a one man shop running open source math/crypto libraries yet I wrote tons of well commented code, 100s of pages of documentation, ran user support, and did all the other things real developers do.  

If they can't add missing ()'s (which aren't strictly needed btw) on their own and merge the code into maintain then they're not much of developers now are they?

> There's a difference between patching an existing file, and
> introducing
> a new one. Should you have introduced a new file based on AH, I
> suspect
> it would also need some cleaning up.

Except when the motto is "use the source luke" you shouldn't be surprised when ... people use the source as a template for getting work done.  If you guys have such a damn problem with this you should maintain your code so it serves as an example to others.  Stop being a hypocrite.

> Anyways, I'll stop feeding the troll now. You've made it quite clear
> that no one is going to convince you, and calling the maintainers
> lazy
> and hypocrites isn't going to convince them.

I'm sure I'll re-write the patch [when I can't say].  I'm not in a position to sell the AH-AEAD work to my bosses because the ROI just won't be there.  For now we and our customers will have access to CMAC and nobody else will which is just a crying shame.

I'm not trying to pick a fight I'm trying to point out flaws in the system.  That you guys are so immobile on it is a shame.  I'd love to sell the idea to my bosses of cleaning up the CryptoAPI/IPsec maybe down the road merging our hardware drivers into mainline but it's not a case I can really make right now.  We make due quite fine with a private GPL tree of code but that's the opposite of how this should work.

Tom

Re: IPsec AH use of ahash

[Steven Rostedt]

On Sun, Jan 20, 2013 at 01:47:01PM -0500, Tom St Denis wrote:
> 
> 
> > 
> > You mean the content at http://lkml.org/lkml/2012/12/11/369 ?
> > 
> > Funny how posting to an mailing list works...
> 
> I posted that in December ... it wasn't till January that I got the first reply back about it failing to meet the coding "standards."  Do you honestly expect people to sit on their hands for a month each time simple requests float by?

You posted in mid December. Guess what, that's a high time for people to
take off for vacations. Also it's a time where a lot of year end work
needs to get done and yes, new code does get pushed to the bottom of the
priority list.

A polite "ping" is allowed if you don't hear back within a week. But
still, this time of year is not the best for new work to be added.

BTW, how much is your company paying David Miller to fix your
submissions?  If you're not paying him, don't expect things to get done
at your convenience, or at all.

> 
> Furthermore, it's not merged into mainline.  So no, you don't have the content.  

Seems nobody (besides you) cares at the moment. When someone comes by
and wants this feature, as you so nicely posted it to LKML, it's out
there. They can then clean it up to David's liking, and it will be
added. There is no harm on our side.

You on the other hand need to constantly send patches to your customers.
Seems that the one being hurt by not including these patches is still
you.

> 
> > >  And the only reason you don't is because someone with pull-request
> > > authority is too lazy [or apathetic] to make minor cosmetic changes
> > > and get the change pushed through the process.
> > 
> > Someone like the author?
> 
> You mean the maintainer right?  If I have to do all of the work I should get all of the credit.

You do get credit for the code that you submit. I have people send me
code for a new feature, and I expect them to still maintain it. They
usually have their name at the top of the file as the author.

But the maintainer of the subsystem has the right to control how things
get added to their system, as they are the ones ultimately responsible
for the code out there. If there's a bug, the maintainer is the one that
gets the notice. That's what they maintain, not cleaning up code from
people that want new features. A maintainer is the one that has to make
sure that the code continues to work.

>   
> > > Furthermore I don't think you get how the business side of this
> > > works.
> > 
> > I've been doing both kernel and proprietary development for close to
> > two
> > decades, so you might be surprised. I see it the other way around --
> > you
> > don't see how the open source part works, and are expecting to
> > substitute your proprietary process -- where crap code that "works"
> > is
> > preferred, to hell with trying to maintain consistency,
> > maintainability,
> > or even readability.
> 
> Um ok that's uncalled for.  What part of the fact my CMAC code is like 90% verbatim copied from the XCBC code do you not understand?

So what?

That code is there. There's a lot of code in the kernel that needs to be
cleaned up. I've done exactly as you did. I copied code, submitted it to
the maintainer, and it was rejected because things have changed since the
time of the code I copied. Did I bitch to the maintainer saying "hey, I
copied this and you should accept it because you have that there." No, I
did not. Actually what I did was fix my patch to what was the right way
to do things, and then I FIXED THE CODE I COPIED!!!


>If you think the CMAC patch was bad then you better call up the authors of the XCBC code and call them out for it too.  Stop being hypocritical.

Oh shut up!

The XCBC code is in there today. And yes, if you submitted patches to
clean that code up, I'm sure those will be accepted. But changing code
in the kernel does take work, including removing said code. But that's
still no reason to allow more code that will need to be cleaned up in
the future. Fix it before it gets in.


> 
> You claimed my code fails to maintain "consistency" ... IT'S COPIED FROM EXISTING CODE.  Your complaint makes no sense not to mention it's insulting.

He's not the one calling people lazy. Frankly sir, you're the one being
offensive and a hypocrite.

> 
> Furthermore, as someone who ran OSS projects (some of which are in millions of devices around the world) I understood to cherish user contributions.  Even if they needed polishing up.  I did the bitch work of documenting, testing, packaging, development, debugging, and support.  It's far far far too common to see OSS developers shy away from the less fun side of software development...

I maintain OSS projects too. And because there's very few people that
contribute, I cherish submissions as well, and will do a lot of the
grunt work that really should be done by the contributor. But the Linux
kernel is much bigger. It changes something like 3000 lines of code a day!

Maintainers do not scale well with such a project, and yes, it's up to
the contributors to take up the slack.


> 
> I called out the maintainers here because because not only do they not maintain the integrity of the code for which they claim authority but they are hostile towards outside contributions with ridiculous standards that they themselves don't even adhere to.  Code that they work on in the kernel doesn't meet ANY of the standards set forth that I was supposed to magically divine applied here..

My God, you sound like a 2 year old.

Suck it up, and do the fixes. It's not the time you are worried about,
as you most definitely spent more time bitching to everyone than it
would have taken you to clean up your work.

>  
> > You sold your boss a bill of goods, then -- that may fly in your
> > shop,
> > but that doesn't work here. It is well known that there is likely to
> > be
> > several rounds of review, and this isn't a new thing.
> 
> Ok sure but why does that involve me re-writing the patch for cosmetic reasons?

Because you are the one that cares about it being submitted. If someone
else cares, they'll take your code and fix it, to get it in.

>I agreed that adding the () around the ^/& expressions would be a good idea.  However, how is it a good use of my time to re-write the patch (which includes lines of code I didn't write in the first place) for coding styles that are not actually used in the kernel?

It's better on your time than the maintainers. Do not try to compare
your little OSS project to the Linux Kernel. They are two different
beasts.

> 
> I **COPIED** existing code.  Yet somehow my code isn't good enough?

And that's the perfect time to clean it up.

> 
> > I do know this, and I also know that the kernel is much better
> > documented than the most of the closed source code bases I've looked
> > at
> > in my career.
> 
> That's not really an excuse for lack of documentation/comments.  

We can always use more documentation and comments. And actually, the
kernel has been getting better at this regard. New code usually does get
better comments than what is currently in the kernel from years past.

> 
> > You think davem is lazy? Gruff, sure, but lazy? Well, thank you for
> > illuminating your grasp of reality.
> 
> I was a one man shop running open source math/crypto libraries yet I wrote tons of well commented code, 100s of pages of documentation, ran user support, and did all the other things real developers do.  
> 
> If they can't add missing ()'s (which aren't strictly needed btw) on their own and merge the code into maintain then they're not much of developers now are they?

Because honestly I doubt Dave cares about it. Again, how much are you
paying him?  You're the one that wants this feature. If someone else
wants it, they'll do this without the 2 year old bitch rant.

> 
> > There's a difference between patching an existing file, and
> > introducing
> > a new one. Should you have introduced a new file based on AH, I
> > suspect
> > it would also need some cleaning up.
> 
> Except when the motto is "use the source luke" you shouldn't be surprised when ... people use the source as a template for getting work done.  If you guys have such a damn problem with this you should maintain your code so it serves as an example to others.  Stop being a hypocrite.

Fix the source Luke! It's not a big deal.

> 
> > Anyways, I'll stop feeding the troll now. You've made it quite clear
> > that no one is going to convince you, and calling the maintainers
> > lazy
> > and hypocrites isn't going to convince them.
> 
> I'm sure I'll re-write the patch [when I can't say].  I'm not in a position to sell the AH-AEAD work to my bosses because the ROI just won't be there.  For now we and our customers will have access to CMAC and nobody else will which is just a crying shame.

And when someone wants it, they will either fix up what you did, or pay
someone to do it for them.

> 
> I'm not trying to pick a fight I'm trying to point out flaws in the system.  That you guys are so immobile on it is a shame.  I'd love to sell the idea to my bosses of cleaning up the CryptoAPI/IPsec maybe down the road merging our hardware drivers into mainline but it's not a case I can really make right now.  We make due quite fine with a private GPL tree of code but that's the opposite of how this should work.

You did a good job at picking a fight here. I don't see a flaw in the
system. I see a way to get new code in at a higher standard than the old
code, and over time fix the old code so that it catches up with what's
there today. This is what kernel janitors was created for.

-- Steve

Re: IPsec AH use of ahash

[Borislav Petkov]

On Sun, Jan 20, 2013 at 05:54:07PM -0500, Steven Rostedt wrote:
> My God, you sound like a 2 year old.
> 
> Suck it up, and do the fixes. It's not the time you are worried about,
> as you most definitely spent more time bitching to everyone than it
> would have taken you to clean up your work.

It's like you're reading my mind... :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Borislav Petkov" <bp@alien8.de>
> To: "Steven Rostedt" <rostedt@goodmis.org>
> Cc: "Tom St Denis" <tstdenis@elliptictech.com>, "David Dillow" <dave@thedillows.org>, linux-kernel@vger.kernel.org,
> netdev@vger.kernel.org
> Sent: Sunday, 20 January, 2013 7:34:41 PM
> Subject: Re: IPsec AH use of ahash
> 
> On Sun, Jan 20, 2013 at 05:54:07PM -0500, Steven Rostedt wrote:
> > My God, you sound like a 2 year old.
> > 
> > Suck it up, and do the fixes. It's not the time you are worried
> > about,
> > as you most definitely spent more time bitching to everyone than it
> > would have taken you to clean up your work.
> 
> It's like you're reading my mind... :-)

The problem is for me to add the ()'s is a no brainer.

For me to re-write complete statements for coding style reasons means I have to actually go out and test it again.  In a proper IPsec lab that can take a couple of hours to make sure it's been run through its paces.  That's [among other things] the problem I have.  Re-writing perfectly good code because it doesn't meet coding "standards" that nobody else seems to match.

If you guys can't appreciate that I should just bow out of the discussion.  I didn't come here looking for a fight I came here to seek reason.

Tom

Re: IPsec AH use of ahash

[Borislav Petkov]

On Sun, Jan 20, 2013 at 07:40:49PM -0500, Tom St Denis wrote:
> If you guys can't appreciate that I should just bow out of the
> discussion. I didn't come here looking for a fight I came here to seek
> reason.

Ok, I'm going to pretty much repeat what Steve said but I'm going to
repeat it only once and if you don't want to understand it then, don't
even bother replying.

If you want to get your code in the kernel, simply do what the
maintainer asks you and stop debating. He's not asking you to swallow
knifes or walk on embers or some other crazy shit - he asks you
reasonable stuff and he *knows* that code needs testing and that it
takes a while but this is a price we all pay. Accept it.

You're not the only one being asked to retest stuff and you're not going
to be the last one so stop wasting your time and get something done
instead.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: IPsec AH use of ahash

[David Dillow]

On Sun, 2013-01-20 at 19:40 -0500, Tom St Denis wrote:
> The problem is for me to add the ()'s is a no brainer.

It was more than just the ()'s. You completely botched the indentation
in the parts of code you didn't copy from xcbc.c, among other issues.

> For me to re-write complete statements for coding style reasons means
> I have to actually go out and test it again.

Well, for some of the code you submitted, that would be the first
testing it got, even compile testing AFAICT. I don't doubt that you
tested against your hardware, but it is obvious that you didn't run the
algorithm tests you added, or even tried to compile them. You were
missing a semicolon after your test data, and you were also missing a
"\x" in there, so your test data was wrong.

I know this, because I just spent the five minutes required to fix up
the checkpatch warnings to prove to myself that you could have done it
faster than sending one considered message to the list about how much we
suck. Of course, that assumes you are trying to have a discussion
instead of trolling; it's much easier to spew bile than to think before
you post.

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "David Dillow" <dave@thedillows.org>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Borislav Petkov" <bp@alien8.de>, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, "Steven Rostedt"
> <rostedt@goodmis.org>
> Sent: Monday, 21 January, 2013 4:18:01 AM
> Subject: Re: IPsec AH use of ahash
> 
> On Sun, 2013-01-20 at 19:40 -0500, Tom St Denis wrote:
> > The problem is for me to add the ()'s is a no brainer.
> 
> It was more than just the ()'s. You completely botched the
> indentation
> in the parts of code you didn't copy from xcbc.c, among other issues.

Leaving coding indentation aside [I assume you mean the use of braces because my tab stops should match given the patchset for the other files was minimal] that's not really a good use of time here.
 
> > For me to re-write complete statements for coding style reasons
> > means
> > I have to actually go out and test it again.
> 
> Well, for some of the code you submitted, that would be the first
> testing it got, even compile testing AFAICT. I don't doubt that you
> tested against your hardware, but it is obvious that you didn't run
> the
> algorithm tests you added, or even tried to compile them. You were
> missing a semicolon after your test data, and you were also missing a
> "\x" in there, so your test data was wrong.

The missing semi-colon is in fact missing and for that I apologize.  This btw is the first legitimate gripe with the code thus far.

Though I must say this code has "compiled" in 3.6, 3.6.7, 3.7, and 3.8 without error so it's not surprising it wasn't caught.  After having consulted the "documentation" I never really did figure out how to get testmgr to run so I took a bit of leap of faith there that everything was fine.  Not really an excuse but it is at least an observable hole in the build documentation.

I couldn't spot the missing \x but I don't doubt you.  That format for vector data is so backwards that it's easy to make that mistake.  After having worked on crypto code for 10+ years I've never seen anyone use that format and I've seen stupid formats before (NIST CAVP anyone?).

Again all valid critiques of the code and valid reasons to do a re-spin.  I apologize for the oversight there.  Having "used the source" I found the build symbol required to activate the test manager so I should be good to go now.

> I know this, because I just spent the five minutes required to fix up
> the checkpatch warnings to prove to myself that you could have done
> it
> faster than sending one considered message to the list about how much
> we
> suck. Of course, that assumes you are trying to have a discussion
> instead of trolling; it's much easier to spew bile than to think
> before
> you post.

This wasn't about how you all suck.  Quite the contrary it was about how good people are missing perspective.  You all started this by harping on the coding style "problems" but as a new developer to the kernel (well at least a new submitter...) I used the source as a guide so that I wouldn't deviate from what I perceived as the norm.  I was legitimately frustrated because performing a re-spin solely to work past coding style problems is not something a business case could be made for.  I'd like to contribute properly but at the same time you can't throw red flags on the play each time someone plays the game like you do.

At least the testmgr errors are something I can work on whenever without setting up a lab so likely that'll be something I can tackle today actually.

Tom

Re: IPsec AH use of ahash

[Steven Rostedt]

On Mon, 2013-01-21 at 05:20 -0500, Tom St Denis wrote:
> 
> ----- Original Message -----
> > From: "David Dillow" <dave@thedillows.org>
> > To: "Tom St Denis" <tstdenis@elliptictech.com>
> > Cc: "Borislav Petkov" <bp@alien8.de>, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, "Steven Rostedt"
> > <rostedt@goodmis.org>
> > Sent: Monday, 21 January, 2013 4:18:01 AM
> > Subject: Re: IPsec AH use of ahash
> > 
> > On Sun, 2013-01-20 at 19:40 -0500, Tom St Denis wrote:
> > > The problem is for me to add the ()'s is a no brainer.
> > 
> > It was more than just the ()'s. You completely botched the
> > indentation
> > in the parts of code you didn't copy from xcbc.c, among other issues.
> 
> Leaving coding indentation aside [I assume you mean the use of braces because my tab stops should match given the patchset for the other files was minimal] that's not really a good use of time here.

But it's a good use of David Miller's time?

$ git log v3.6..v3.7 |grep 'Signed-off-by: David.*Miller' | wc -l
679

$ git log v3.6..v3.7 |grep 'Author: David.*Miller' | wc -l
136

David handled 679 patches, where 136 of them were his. This is between
3.6 and 3.7 which is a 3 month period. He handled 543 patches that were
not his. Imagine if he had to fix up coding styles for every one of
those patches. That would make any sane man quit their job.

I'm curious, how many patches do you handle for your OSS projects over a
3 month period?

>  
> > > For me to re-write complete statements for coding style reasons
> > > means
> > > I have to actually go out and test it again.
> > 
> > Well, for some of the code you submitted, that would be the first
> > testing it got, even compile testing AFAICT. I don't doubt that you
> > tested against your hardware, but it is obvious that you didn't run
> > the
> > algorithm tests you added, or even tried to compile them. You were
> > missing a semicolon after your test data, and you were also missing a
> > "\x" in there, so your test data was wrong.
> 
> The missing semi-colon is in fact missing and for that I apologize.  This btw is the first legitimate gripe with the code thus far.

I've also found that those that have poor coding styles also have more
of these "legitimate" problems too. Which is another reason not to
accept patches with coding style issues.

> 
> Though I must say this code has "compiled" in 3.6, 3.6.7, 3.7, and 3.8 without error so it's not surprising it wasn't caught.  After having consulted the "documentation" I never really did figure out how to get testmgr to run so I took a bit of leap of faith there that everything was fine.  Not really an excuse but it is at least an observable hole in the build documentation.

Did you try different configs? Before submitting my code, I run under 8
different configs. SMP, non-SMP, PREEMPT, non PREEMPT, etc. also I run a
few random configs before submitting.

> 
> I couldn't spot the missing \x but I don't doubt you.  That format for vector data is so backwards that it's easy to make that mistake.  After having worked on crypto code for 10+ years I've never seen anyone use that format and I've seen stupid formats before (NIST CAVP anyone?).
> 
> Again all valid critiques of the code and valid reasons to do a re-spin.  I apologize for the oversight there.  Having "used the source" I found the build symbol required to activate the test manager so I should be good to go now.

Until you can play by the rules, don't bother playing. Feel free to dump
code on LKML. Maybe someone that knows how to play the game will take
it. But leave your gripes to your managers. No one here, but you, thinks
there's a problem with the frame work.

> 
> > I know this, because I just spent the five minutes required to fix up
> > the checkpatch warnings to prove to myself that you could have done
> > it
> > faster than sending one considered message to the list about how much
> > we
> > suck. Of course, that assumes you are trying to have a discussion
> > instead of trolling; it's much easier to spew bile than to think
> > before
> > you post.
> 
> This wasn't about how you all suck.  Quite the contrary it was about how good people are missing perspective.  You all started this by harping on the coding style "problems" but as a new developer to the kernel (well at least a new submitter...) I used the source as a guide so that I wouldn't deviate from what I perceived as the norm.  I was legitimately frustrated because performing a re-spin solely to work past coding style problems is not something a business case could be made for.  I'd like to contribute properly but at the same time you can't throw red flags on the play each time someone plays the game like you do.

> At least the testmgr errors are something I can work on whenever without setting up a lab so likely that'll be something I can tackle today actually.

While you're at it. You could spend an extra 5 minutes cleaning up the
coding style ;-)

-- Steve

Re: IPsec AH use of ahash

[Tom St Denis]

----- 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 8:38:54 AM
> Subject: Re: IPsec AH use of ahash

> > The missing semi-colon is in fact missing and for that I apologize.
> >  This btw is the first legitimate gripe with the code thus far.
> 
> I've also found that those that have poor coding styles also have
> more
> of these "legitimate" problems too. Which is another reason not to
> accept patches with coding style issues.

I find that 73% of all stats are made up.

What actually happened was testmgr.h was being #ifdef'ed out so despite the fact I saw "testmgr.o" on the build process it wasn't actually doing anything.  Not blaming anyone here .. just missed a build option.

I wouldn't have submitted it if the code failed to compile.

> > Though I must say this code has "compiled" in 3.6, 3.6.7, 3.7, and
> > 3.8 without error so it's not surprising it wasn't caught.  After
> > having consulted the "documentation" I never really did figure out
> > how to get testmgr to run so I took a bit of leap of faith there
> > that everything was fine.  Not really an excuse but it is at least
> > an observable hole in the build documentation.
> 
> Did you try different configs? Before submitting my code, I run under
> 8
> different configs. SMP, non-SMP, PREEMPT, non PREEMPT, etc. also I
> run a
> few random configs before submitting.

None of course apply to this code [or shouldn't at least].  

> Until you can play by the rules, don't bother playing. Feel free to
> dump
> code on LKML. Maybe someone that knows how to play the game will take
> it. But leave your gripes to your managers. No one here, but you,
> thinks
> there's a problem with the frame work.

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

I'll avoid beating the dead horse though I'd like to move forward.

> > At least the testmgr errors are something I can work on whenever
> > without setting up a lab so likely that'll be something I can
> > tackle today actually.
> 
> While you're at it. You could spend an extra 5 minutes cleaning up
> the
> coding style ;-)

It actually took me about 45 minutes and about 5 revisions of the patches to clean up all the random coding style gripes from checkpatch.  The only reason I worked on it though is that there were build errors.  That way I can justify it to my boss.

Seriously, no spaces on the trailing edge of multi line comments?  :-/

Anyways, I did re-submit.  I still have no idea how testmgr works but hopefully someone can pick it up from there.

Tom

Re: IPsec AH use of ahash

[Steven Rostedt]

On Mon, 2013-01-21 at 08:45 -0500, Tom St Denis wrote:
> 
> ----- 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 8:38:54 AM
> > Subject: Re: IPsec AH use of ahash
> 
> > > The missing semi-colon is in fact missing and for that I apologize.
> > >  This btw is the first legitimate gripe with the code thus far.
> > 
> > I've also found that those that have poor coding styles also have
> > more
> > of these "legitimate" problems too. Which is another reason not to
> > accept patches with coding style issues.
> 
> I find that 73% of all stats are made up.

I was only talking about my own experience. I gave no numbers.


> > Did you try different configs? Before submitting my code, I run under
> > 8
> > different configs. SMP, non-SMP, PREEMPT, non PREEMPT, etc. also I
> > run a
> > few random configs before submitting.
> 
> None of course apply to this code [or shouldn't at least].

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

>   
> 
> > Until you can play by the rules, don't bother playing. Feel free to
> > dump
> > code on LKML. Maybe someone that knows how to play the game will take
> > it. But leave your gripes to your managers. No one here, but you,
> > thinks
> > there's a problem with the frame work.
> 
> I actually did resubmit this morning with most of the checkpatch issues fixed. 

Thank you.

> 
> I'll avoid beating the dead horse though I'd like to move forward.
> 
> > > At least the testmgr errors are something I can work on whenever
> > > without setting up a lab so likely that'll be something I can
> > > tackle today actually.
> > 
> > While you're at it. You could spend an extra 5 minutes cleaning up
> > the
> > coding style ;-)
> 
> It actually took me about 45 minutes and about 5 revisions of the patches to clean up all the random coding style gripes from checkpatch.  The only reason I worked on it though is that there were build errors.  That way I can justify it to my boss.

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

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

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.

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.

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

-- Steve

Re: IPsec AH use of ahash

[Tom St Denis]

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

Re: IPsec AH use of ahash

[Steven Rostedt]

On Mon, 2013-01-21 at 09:51 -0500, Tom St Denis wrote:
> 
> ----- 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.

Heh, I got it. My point was that sloppy coding usually comes with
mistakes. I wasn't saying that your code is sloppy, or that all code
that checkpatch fails on is sloppy. But it's a way to clean things up.

Actually, there's been several times I found that cleaning up code that
checkpatch told me to fix, I would find a bug. Usually a silly error,
but something that would have needed to be fixed.

> 
> Let it go.

It's gone.

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

It's also up to the maintainer. Some maintainers are stricter than
others. And if they find that checkpatch is helpful, they may be more
inclined to push harder.

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

The maintainer does.

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

The differences are really minor. And yes, sometimes you don't find out
until you send a patch to the maintainer. I've had to change patches to
others code because of this. But really, I never thought it was a big
deal. As a maintainer myself, I know that I want the code that I
maintain to be a certain way, because that makes me more efficient in
understanding the code. And I need to understand code that I didn't
write.

When I send a patch to another maintainer, and they tell me to fix the
way I did the comments, I don't complain. I fix the comments and resend.

-- Steve

Re: IPsec AH use of ahash

[Tom St Denis]

----- 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 10:28:33 AM
> Subject: Re: IPsec AH use of ahash
> 
> When I send a patch to another maintainer, and they tell me to fix
> the
> way I did the comments, I don't complain. I fix the comments and
> resend.

Which is less of a problem when there is a timeliness factor.  In the business world people move on and don't work at that pace.

Anyways, neither here nor there now.  

Tom

Re: IPsec AH use of ahash

[Chris Friesen]

On 01/21/2013 09:31 AM, Tom St Denis wrote:
> ----- 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 10:28:33 AM Subject: Re: IPsec AH use of ahash
>>
>> When I send a patch to another maintainer, and they tell me to fix
>> the way I did the comments, I don't complain. I fix the comments
>> and resend.
>
> Which is less of a problem when there is a timeliness factor.  In the
> business world people move on and don't work at that pace.

There can be an impedance mismatch between the "get it done to hit a 
deadline" business world and the "get it right no matter how long it 
takes" world of some open-source projects.

However, many businesses have recognized that they get far more benefit 
from dealing with open-source than it costs them in designer time.

 From the point of view of my employer (I work in telecom) the choices 
are either:

a) work with the kernel to get the code submitted into mainline
b) keep the changes private and port them every time we upgrade

Over a decade or more my management has come to realize that option "a" 
is generally better in the long term, even if it's a bit more effort in 
the short term.

There are exceptions of course, and sometimes we just need to do a 
quick-and-dirty solution to get something out the door.

Chris

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Chris Friesen" <chris.friesen@genband.com>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Steven Rostedt" <rostedt@goodmis.org>, "David Dillow" <dave@thedillows.org>, "Borislav Petkov" <bp@alien8.de>,
> linux-kernel@vger.kernel.org, netdev@vger.kernel.org
> Sent: Monday, 21 January, 2013 10:49:19 AM
> Subject: Re: IPsec AH use of ahash
> 
> On 01/21/2013 09:31 AM, Tom St Denis wrote:
> > ----- 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 10:28:33 AM Subject: Re: IPsec AH use of ahash
> >>
> >> When I send a patch to another maintainer, and they tell me to fix
> >> the way I did the comments, I don't complain. I fix the comments
> >> and resend.
> >
> > Which is less of a problem when there is a timeliness factor.  In
> > the
> > business world people move on and don't work at that pace.
> 
> There can be an impedance mismatch between the "get it done to hit a
> deadline" business world and the "get it right no matter how long it
> takes" world of some open-source projects.

I think part of it is the nature of the job.  Here working with the kernel is a by-product of the atmosphere we're in (embedded hardware).  We're not per se a "Linux Shop" ...

So for our case when CMAC code was working and the patch fired off we were "done."  Granted now that bugs were found in the testmgr code spending time to fix it up was prudent.

The timeliness I was speaking of was to make sure that business windows are hit as well.  Had I had a rejection sooner when the project was still fresh and current it would have been no issue to re-spin the patches.
 
> However, many businesses have recognized that they get far more
> benefit
> from dealing with open-source than it costs them in designer time.
> 
>  From the point of view of my employer (I work in telecom) the
>  choices
> are either:
> 
> a) work with the kernel to get the code submitted into mainline
> b) keep the changes private and port them every time we upgrade
> 
> Over a decade or more my management has come to realize that option
> "a"
> is generally better in the long term, even if it's a bit more effort
> in
> the short term.
> 
> There are exceptions of course, and sometimes we just need to do a
> quick-and-dirty solution to get something out the door.

Yup agreed.  We don't plan to mainline our hardware drivers any time soon mostly because they're in flux too much and they're really only useful for businesses who order mostly custom designs (end home users wouldn't typically write drivers for them).

But at the same time we hit upon things in the kernel that prevent our private tree drivers from working (lacking CMAC for instance, or not being able to run AH-GMAC).  I'd like to see the CMAC issue move forward and then maybe contribute to the AH-AEAD front.

Tom

Re: IPsec AH use of ahash

[Alan Cox]

Look at it from the kernel end. What happens if your change shows up bugs on another architecture or has a flaw. It works for you now but you plan to dump and run. That's not a viable long term development model for upstream.

The licence allows you to do it, and other parties who care more to pick it up and run with it. Unless someone does however it's just a burden. If nobody wants it upstream enough better it stays out perhaps - if the call is wrong eventually other people will care enoug to share the work. If not you get to pick between doing the extra or re-porting your code to new releases 

Alan

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
> To: "Tom St Denis" <tstdenis@elliptictech.com>, "David Dillow" <dave@thedillows.org>
> Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org
> Sent: Sunday, 20 January, 2013 3:30:49 PM
> Subject: Re: IPsec AH use of ahash
> 
> Look at it from the kernel end. What happens if your change shows up
> bugs on another architecture or has a flaw. It works for you now but
> you plan to dump and run. That's not a viable long term development
> model for upstream.

Alan, I'm really not trying to be rude but if you look at the depth of the patch I was submitting ... it's not architecture specific, it's 90% copied from another source file that is currently in the tree, I have actually tested it on x86_32/arm already as it is, etc.  I agree that more core pieces of architecture specific code would require tighter scrutiny but this is hardly the case here. 

> The licence allows you to do it, and other parties who care more to
> pick it up and run with it. Unless someone does however it's just a
> burden. If nobody wants it upstream enough better it stays out
> perhaps - if the call is wrong eventually other people will care
> enoug to share the work. If not you get to pick between doing the
> extra or re-porting your code to new releases

Realistically, the CryptoAPI maintainers should have added CMAC long ago.  I mean they sought fit to add non-standard modes like VMAC or Serpent [or Twofish or ...] but not a NIST standard like CMAC which is used in RFCs for IPsec [among other things]?  Are they writing crypto for Kernel users or is this their pet project where they get to hack together random bits of ciphering what not for their own amusement?

Sure few people are clamouring to use CMAC over say HMAC for MAC generation but it is a standard and it's prevalent enough that supporting it is a good idea.

Forcing me to go through a new testing cycle just because you'd prefer

if (expr)
   foo;

Instead of 

if (expr) {
   foo;
}

or 

err = expr;
if (err) ...

instead of 

if ((err = expr)) ...

is not a good use of anyones time.  And it's hypocritical given that I sourced that coding style from code that is already in the kernel.

Tom

Re: IPsec AH use of ahash

[H. Peter Anvin]

On 01/20/2013 07:07 AM, Tom St Denis wrote:
>
> In all likelihood I will submit a revised CMAC patch but it'll take
> time before I can get business hours to work on it.  So instead of
> having a maintainer just touch it up we're all going to lose out
> because of pride?
>

It's not about pride.  It is about the fact that maintainers don't 
scale.  A single troublesome contributor can easily take up as much 
maintainer time as over a dozen contributors who know how to work well 
with their upstream.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: IPsec AH use of ahash

[Tom St Denis]

----- Original Message -----
> From: "H. Peter Anvin" <hpa@zytor.com>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Mike Galbraith" <bitbucket@online.de>, "Eric Dumazet" <erdnetdev@gmail.com>, "Waskiewicz Jr, Peter P"
> <peter.p.waskiewicz.jr@intel.com>, "David Miller" <davem@davemloft.net>, "steffen klassert"
> <steffen.klassert@secunet.com>, herbert@gondor.hengli.com.au, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
> "Michal Kubecek" <mkubecek@suse.cz>
> Sent: Sunday, 20 January, 2013 12:03:34 PM
> Subject: Re: IPsec AH use of ahash
> 
> On 01/20/2013 07:07 AM, Tom St Denis wrote:
> >
> > In all likelihood I will submit a revised CMAC patch but it'll take
> > time before I can get business hours to work on it.  So instead of
> > having a maintainer just touch it up we're all going to lose out
> > because of pride?
> >
> 
> It's not about pride.  It is about the fact that maintainers don't
> scale.  A single troublesome contributor can easily take up as much
> maintainer time as over a dozen contributors who know how to work
> well
> with their upstream.

Ironically I'd view consistency with existing code as paramount over [say] adherence to some coding standard that none of the code I've seen in the kernel apparently sticks to in the first place.  In this case since XCBC and CMAC operate almost identically it made sense to me to copy it as a template.  Now you're telling me I have to re-write it... so that now it's different than XCBC?  Or are you suggesting that I also re-write XCBC?

Similarly AH4 and AH6 violate the coding standards.  Are you suggesting I re-write those entirely as well to merely augment its functionality?

For a project that *boasts* about it's abhorrent lack of commenting/documentation since "the source is the documentation" it's funny that you can't actually READ the source as an authority.

Tom