review of the backupkey protocol implementation

Andrew Bartlett abartlet at samba.org
Mon Oct 11 18:06:43 MDT 2010


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?

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

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20101012/967e1bec/attachment.pgp>


More information about the samba-technical mailing list