review of the backupkey protocol implementation

[Matthieu Patou]

Metze,

Can I push the version that is in your branch it seems that you are 
quite busy, andrew found the code quite correct and you already reviewed 
the code so it should be basically of a correct quality. In the same 
time I've to maintain out of tree branch and to keep an eye on it when I 
would like to concentrate on other stuff.

Matthieu.

On 02/02/2011 01:48, Matthieu Patou wrote:
> 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