[PATCH] DsReplGetInfo() - infoType == DS_REPL_INFO_NEIGHBORS

[Erick Nogueira do Nascimento]

Hi Tridge,

Thanks for the advices, I will follow them from now on.

> Thanks. For this sort of patch, it is best to split it up into logical
> pieces. In this case I think your smbtorture work should be a separate
> patch from your dsdb/kcc work, and your two dsdb/kcc changes could
> also be better split. I think one patch adding the framework for the
> kccdrs_replica_get_info call, and one for each of the info levels
> would make sense. If you are not sure how to split up patches, then
> perhaps the easiest is to take the code as a whole and use "git add
> --patch" then follow the prompts for what you want to include in the
> current commit. After you've included what you want in a commit then
> use "git commit" to add it.

I split my patch in two: one for the dsdb/kcc work and another for the
torture. I didn't make the split of dsdb/kcc on framework and infoLevel
because that would take some work to reedit the files extracting what
was specific for the DS_REPL_INFO_NEIGHBORS infolevel, remaining only
the patch for the framework, and then applying the infolevel specific
code and creating another patch.
I think the code is clear enough to understand where the code for each
info level must be placed, so I think this is not a major issue. 
For the next patches, i will follow your advice and write the code with
small patches in mind, from the beginning.

>  - the patch puts in a lot of debug code at a low logging level. It is
>    useful to have this while developing the code, but its a bit too
>    verbose for production code.
> 
>  - the big repsFromTo1 comment at the top of kcc_drs_replica_info.c is
>    not really appropriate in a patch 
> 
>  - you need to check for more failures, for example memory allocation
>    failures, and return an appropriate error code
> 
>  - some of your line lengths are far too long, and the indentation is
>    very bad (eg. in the function header of
>    kccdrs_replica_get_info_neighbours()) (see the samba coding guide
>    for details)
> 
>  - rather than including pieces of the MS-DRSR doc, please just use a
>    reference to the section number and doc name
> 
>  - please make functions static when they are used only by one file
>    (eg. get_master_ncs)
> 
>  - in torture/rpc/dsgetinfo.c, please put your own copyright at the
>    top, but say which other file the code is based on

I made the above changes. About the line lengths, the larger ones are
much smaller now, but there are some above the 80 characters limit
because I didn't find an elegant way to split them. I think their
lengths are now coherent with other parts of samba.

Best Regards,
Erick Nogueira do Nascimento

On Mon, 21 Dec 2009 21:56:23 +1100
tridge at samba.org wrote:

> Hi Erick,
> 
> Thanks for the patch!
> 
>  > This is the patch for the implementation of the DsGetReplInfo()
>  > MS-DRSR 4.1.13) for the case infoType == DS_REPL_INFO_NEIGHBORS
>  > (request information about the repsFrom neighbors of the target
>  > DC). There is also a torture test for this call.
> 
> Thanks. For this sort of patch, it is best to split it up into logical
> pieces. In this case I think your smbtorture work should be a separate
> patch from your dsdb/kcc work, and your two dsdb/kcc changes could
> also be better split. I think one patch adding the framework for the
> kccdrs_replica_get_info call, and one for each of the info levels
> would make sense. If you are not sure how to split up patches, then
> perhaps the easiest is to take the code as a whole and use "git add
> --patch" then follow the prompts for what you want to include in the
> current commit. After you've included what you want in a commit then
> use "git commit" to add it.
> 
>  > On the tests I made using smbtorture (with RPC-DSGETINFO test) it
>  > looks like the values on the fields of the reply message are
>  > consistent with what Windows 2008 fill in those fields.
> 
> good!
> 
>  > I'm planning to send one patch for each of the infoTypes. I can't
>  > implement some of them now, because the information needed is
>  > not available yet on drs. The next patch will be for infoType
>  > DS_REPL_INFO_REPSTO.
> 
> that sounds good, but please also do a bit of cleanup of your current
> patches so they can be included. Apart from the patch reorganisation I
> mentioned above, I think the following things could be improved:
> 
>  - the patch puts in a lot of debug code at a low logging level. It is
>    useful to have this while developing the code, but its a bit too
>    verbose for production code.
> 
>  - the big repsFromTo1 comment at the top of kcc_drs_replica_info.c is
>    not really appropriate in a patch 
> 
>  - you need to check for more failures, for example memory allocation
>    failures, and return an appropriate error code
> 
>  - some of your line lengths are far too long, and the indentation is
>    very bad (eg. in the function header of
>    kccdrs_replica_get_info_neighbours()) (see the samba coding guide
>    for details)
> 
>  - rather than including pieces of the MS-DRSR doc, please just use a
>    reference to the section number and doc name
> 
>  - please make functions static when they are used only by one file
>    (eg. get_master_ncs)
> 
>  - in torture/rpc/dsgetinfo.c, please put your own copyright at the
>    top, but say which other file the code is based on
> 
> These may all seem like minor things, but it helps us to keep the code
> base a bit neater and easier for others to follow. Thanks!
> 
> Cheers, Tridge