review of the backupkey protocol implementation

[Stefan (metze) Metzmacher]

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...

metze

> I have some comments on 's4 idl: Add IDL for backup key protocol':
> 
> Your IDL still has a lot of development artifacts in it:
>  - // comments
>  - magic values - as this is a documented protocol, are these really
> 'magic', or do they have a name?
> 
> in ndr_backupkey.c why do you need to (conditionally) include param.h?
> Please also describe (in comments) why this needs to be manually parsed,
> and what the various elements are, when they are just magic values.
> 
> You seem to have added ndr_backupkey_print.h - but this claims to have
> been an autogenerated prototype file, but clearly you have edited it (//
> comments). 
> 
> Also, in: s4 torture: add new rpc torture tests for backup key remote
> protocol
> 
> Please remove the commented out code, or leave it in at a high debug
> level.  Please remove // comments, and review our coding style (in
> particular around function calls:
> 
> static struct dom_sid *get_user_sid(struct torture_context *tctx, struct
> dcerpc_pipe *p, TALLOC_CTX *mem_ctx, const char* user) {
> 
> should be
> 
> static struct dom_sid *get_user_sid(struct torture_context *tctx, struct
> dcerpc_pipe *p, TALLOC_CTX *mem_ctx, const char* user)
> {
> 
> I hope this helps,
> 
> Andrew Bartlett
> 

-------------- 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/20101012/6e27a4de/attachment.pgp>