review of the backupkey protocol implementation

[Matthieu Patou]

On 12/10/2010 04:06, Andrew Bartlett wrote:
> 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
>
> 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?
No they don't have a name, it's just remarkable values that you have 
like "magic" at the head of binary file to understand which file type it is.
It's used to check if you have a sensible decryption, as you are able to 
find at a given position a known value.
> 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.
I'll have to dig, but I remember that we did this with metze to go over 
a autogenerated code pb.
> 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).
Ok.
> 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)
> {
>
Ok


Thanks.


-- 
Matthieu Patou
Samba Team        http://samba.org