need codereview for krb consortium masterkey keytab stash project
William.Fiveash at Sun.COM
Tue Jul 15 14:25:05 EDT 2008
I've committed the code changes based on your review to the mkey_keytab
On Tue, Jul 15, 2008 at 12:40:26PM -0500, Will Fiveash wrote:
> On Mon, Jul 14, 2008 at 04:39:24PM -0400, Ken Raeburn wrote:
> > Mostly small stuff...
> > The DAL API is changed, but I think that's still an internal-only interface
> > so not doing version checks is okay.
> That's what I was assuming.
> > KADM5_CONFIG_KVNO will need to be reassigned when merging; the iprop merge
> > used up that number. That'll be trivial.
> > It may be my viewer, but some of the indentation in dump.c doesn't seem to
> > match MIT's code style (indenting function arguments on continuation lines
> > to line up with the first argument).
> > Function definitions shouldn't have spaces between the parens and the
> > arguments.
> The problem I have is trying to decide what is correct. If you look at
> the definition of kdb_def_set_mkey() (which I did not modify) in
> kdb_default.c you'll see a space between the parens and the args.
> I've gone ahead and modified the formatting to remove the space between
> the parens and the args for the functions I modified.
> In general I was told by Tom that he preferred a C-style where
> indentation was 4 columns and spaces are used instead of hard tabs.
> This was what I was using however I understand your point about not
> introducing unnecessary diffs so I've gone back and fixed the obvious
> places where the formatting was different.
> > kdb_default.c: On my Solaris system, mktemp(3C) is documented as wanting a
> > string with six trailing Xs; your template string has five. (The Mac
> > version says it'll take any number, but I've seen six mentioned elsewhere.)
> Fixed this to use six X's.
> > The new keytab code should be exercised by the test suite; do you have a
> > test to verify that an old-format stash file will still work?
> Not yet but I will. Can this be postponed?
> Will Fiveash
> Sun Microsystems Inc.
Sun Microsystems Inc.
More information about the krbdev