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