review of the backupkey protocol implementation

[Matthieu Patou]

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 Patou
Samba Team        http://samba.org