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.
>
> 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)
>
> The '{' of functions should be in a new line.
>
> 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...

This should be fixed in the current version of this branch on my private 
repo: 
http://git.samba.org/?p=mat/samba.git;a=shortlog;h=refs/heads/backupkey_heimdal

Can I have your final comment before I push it to autobuild ?

-- 
Matthieu Patou
Samba Team        http://samba.org
Private repo      http://git.samba.org/?p=mat/samba.git;a=summary