review of the backupkey protocol implementation

[Stefan (metze) Metzmacher]

Am 21.11.2010 15:40, schrieb 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

From the pure reading it looks much better formated that the first version

There's a lot more review needed...

See
http://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-tmp3
for a start.

I fear there're a lot of memory leak style bugs hidding
and there're also some misformated stuff in there.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20101122/e11e00dd/attachment.pgp>