Review on patches for multi-ip handling

[Matthieu Patou]

On 11/11/2011 04:30, Andrew Tridgell wrote:
> Hi Matthieu,
>
> I've had a look at these patches, and the others in drs_timebased.
>
> The connect_multi_next_socket() changes look good to me.
>
> It would probably be even clearer to setup an array of structures where
> each structure contains an address and port, then just increment an
> index into that array. That would avoid the slightly confusing division
> needed to work out the right port for a given value of 'next'. I think
> however that the code as you have written it is correct, so if you want
> to put the patch in as it is then that's fine.
Well I'm not too conviced about the need for the structure, so instead I 
added an dedicated index for the port in order to avoid the division 
while keeping things simple.
>
> The "s4-librpc: do not limit to the first IP when trying to do a rpc
> connection" also looks good to me. I'm surprised we haven't had more
> problems with this!
Well in order to trigger the problem it was needed that your samba was 
only listenning on 1 interface with bind interface only = yes.
I'm sure it's a pretty rare setup so far.
>
> The DRS_CRITICAL_ONLY changes are probably OK, but I am a bit concerned
> about how they will affect dbcheck. When we do critical only replication
> we end up with DN links that point to non-existant objects. Andrew and I
> deliberatly didn't use critical-only initial replication because of this
> problem.
No impact on the structure, I ran dbcheck on a joined database.
>
> With the recent changes to DN link handling this may now be all OK, but
> please check that dbcheck doesn't give any errors after a domain join
> with these changes.
>
> The DRS_GET_ANC changes and the ldb_msg_find_attr_as_* changes look good
> to me.
>
> The timeout handling changes in getncchanges could be a bit neater I
> think. I've pushed an alternative implementation to my drs_timebased
> branch for you to take a look at. It adds a flag to the existing
> get_nc_changes_build_object() call instead of having a new
> function.  (I haven't tested it, sorry)
Ok I'll have a look it seems that my solution works so it's a good news. 
I want a couple of more review before pushing those patches still.

Thanks for the review.
Matthieu.



-- 
Matthieu Patou
Samba Team
http://samba.org