[17149] in Kerberos_V5_Development
Re: NSS for PKINIT, in-progress patches available, feedback sought
daemon@ATHENA.MIT.EDU (Nalin Dahyabhai)
Thu Sep 8 18:02:51 2011
Date: Thu, 8 Sep 2011 18:02:36 -0400
From: Nalin Dahyabhai <nalin@redhat.com>
To: Greg Hudson <ghudson@mit.edu>
Message-ID: <20110908220236.GB6229@redhat.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <1315505801.718.47.camel@t410>
Cc: krbdev@mit.edu
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: krbdev-bounces@mit.edu
On Thu, Sep 08, 2011 at 02:16:41PM -0400, Greg Hudson wrote:
> Some quick reactions:
>
> * The configure.in error message says 3.12.11 is required, but the code
> only checks for 3.12.10 or later.
I was dithering on what version number to use there because 3.12.11
isn't released yet -- it should consistently ask for 3.12.11, for now at
least.
> * In Makefile.in, avoid using direct @ substitutions inside variable
> expansions; define intermediate variables like PKINIT_CRYPTO_IMPL_LIBS.
> This allows more flexibility when building, and also means fewer changes
> if the code ever gets to be built on Windows.
Ok, fixing this. The variables will go into pre.in, with others.
> * If pkinit_crypto_nss.c doesn't need anything from k5-int.h, use
> k5-platform.h instead.
Yup, looks like you're right there, too. Fixing.
> * Our language platform assumption is C89, not C99. C99 features found
> in MSVC (like variadic macros) are probably okay, although we haven't
> formally decided that. Other C99 features (like named structure field
> initializers) will be an impediment to using this code on Windows--which
> seems a bit of a shame as I think NSS has better Windows support than
> OpenSSL.
That's a shame, I really like the field initializers. Removing them.
> * Style conformance generally looks good, with a few exceptions:
>
> - There are some >79-character lines. Some are unavoidable because
> NSS uses absurdly long names; others can be reasonably broken up.
>
> - (*fptr) (args) should be (*fptr)(args).
>
> - We normally put a blank line after variable declarations. This
> doesn't seem to be a written rule, though, so I suppose addressing this
> is optional.
>
> - We often omit braces around single-line flow control bodies. Also
> not a written rule, and I know opinions vary on this, so addressing this
> is even more optional.
I've taken a pass through it and tried to clean these up some.
> I will give this a closer review for obvious logic errors and will also
> build the NSS trunk and run this through our test suite (which minimally
> exercises PKINIT). I probably won't be able to give it a terribly
> thorough functional review since there are still a lot of gaps in my
> understanding of the primitives used by PKINIT. But it sounds like
> you've put this through some pretty careful testing, and it's opt-in
> code, so I don't consider that an impediment to committing it.
Thanks, I'm glad for any feedback you can provide.
Nalin
_______________________________________________
krbdev mailing list krbdev@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev
