review of the backupkey protocol implementation

[Matthieu Patou]

Metze, Andrew,

Here is the last 
version:http://git.samba.org/?p=mat/samba.git;a=shortlog;h=refs/heads/bkrp

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

Thanks.

Matthieu

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:
>>> 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
Private repo      http://git.samba.org/?p=mat/samba.git;a=summary