review of the backupkey protocol implementation

Matthieu Patou mat at
Tue Feb 1 15:48:18 MST 2011

Metze, Andrew,

Here is the last 

I removed the gendb_search (worked with the help ldb_search, thks andrew 
for the info) and add comments.

Would be great if the final review was done ... it's been a while since 
this around ....



On 12/10/2010 13:09, Matthieu Patou wrote:
> 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:
>> 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
Private repo;a=summary

More information about the samba-technical mailing list