review of the backupkey protocol implementation

Matthieu Patou mat at
Tue Oct 12 03:05:52 MDT 2010

On 12/10/2010 11:24, Stefan (metze) Metzmacher wrote:
> Am 12.10.2010 02:06, schrieb Andrew Bartlett:
>> 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:
> Please fix the indentation in a lot of places, e.g. the backupkey.idl
> And remove unused stuff.
> Why do we need bkrp_BackupKey_debug? If we really need that it should
> have the 'noopnum'
> property.
I'd like to keep the _debug for the ndrdump as the type is defined by a 
guid, I didn't pushed (yet) the effort on what we talked at SDC.
If I manage to make ndrdump work correctly with your technic I'll trash 
the _debug.
If noopnum then I'm not able to use it with ndrdump.
> Please create a separate commit that removes the old
> protected_storage.idl and related stuff.
> I think we also don't need the ndr_backupkey_print.[ch] files they can
> be merged into ndr_backupkey.[ch]
> What is BIGNUM ? the '*' of pointers should be next to the variable and
> not the type (in a lot of places)
Ok for the *, for the BIGNUM it's a type from heimdal.
> The '{' of functions should be in a new line.
I thought it was like if/for, I'll check the coding guideline
> char *addr = "unknown"; should be 'const char *addr = "unknown"'
> You don't need sub_ctx and explicit talloc_free/unlink calls, mem_ctx
> is already the temporary memory context, which is cleaned up after the call.
> And please go through each single line and fix the identation and coding
> style,
> it's really not that hard...
I was aware for the style I didn't really retouched the file since we 
left redmond ...
Was asking for a review on the server code to see if it's globally correct.


Matthieu Patou
Samba Team

More information about the samba-technical mailing list