review of the backupkey protocol implementation

[simo]

On Tue, 2010-10-12 at 13:05 +0400, Matthieu Patou wrote:
> On 12/10/2010 11:24, Stefan (metze) Metzmacher wrote:
> > Am 12.10.2010 02:06, schrieb Andrew Bartlett:
> >> On Tue, 2010-10-05 at 10:43 +0400, Matthieu Patou wrote:
> >>> Hello,
> >>>
> >>> I would like to have a code review of my implementation of the backupkey
> >>> protocol.
> >>>
> >>> The whole stuff is working correctly now and pass make test but I think
> >>> it would be great to have a review just in case there is some code smell.
> >>>
> >>> It's here:
> >>> http://git.samba.org/?p=mat/samba.git;a=shortlog;h=refs/heads/backupkey_heimdal_recent
> > Please fix the indentation in a lot of places, e.g. the backupkey.idl
> >
> > And remove unused stuff.
> >
> > Why do we need bkrp_BackupKey_debug? If we really need that it should
> > have the 'noopnum'
> > property.
> I'd like to keep the _debug for the ndrdump as the type is defined by a 
> guid, I didn't pushed (yet) the effort on what we talked at SDC.
> If I manage to make ndrdump work correctly with your technic I'll trash 
> the _debug.
> If noopnum then I'm not able to use it with ndrdump.
> > Please create a separate commit that removes the old
> > protected_storage.idl and related stuff.
> >
> > I think we also don't need the ndr_backupkey_print.[ch] files they can
> > be merged into ndr_backupkey.[ch]
> >
> > http://git.samba.org/?p=mat/samba.git;a=commitdiff;h=36269ec9dec917c7500e73ca69fa51149b768d0c
> > What is BIGNUM ? the '*' of pointers should be next to the variable and
> > not the type (in a lot of places)
> Ok for the *, for the BIGNUM it's a type from heimdal.
> > The '{' of functions should be in a new line.
> I thought it was like if/for, I'll check the coding guideline
> > char *addr = "unknown"; should be 'const char *addr = "unknown"'
> >
> > You don't need sub_ctx and explicit talloc_free/unlink calls, mem_ctx
> > is already the temporary memory context, which is cleaned up after the call.
> >
> > And please go through each single line and fix the identation and coding
> > style,
> > it's really not that hard...
> I was aware for the style I didn't really retouched the file since we 
> left redmond ...
> Was asking for a review on the server code to see if it's globally correct.
> 
> Matthieu.

Matthieu, any specific reason why you are using internal Heimdal x509
functions instead of just using an SSL library like openSSL, NSS,
GNUTLS, etc.. ?

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>