[Freeipa-devel] [PATCH 0014] correct handling of one directional segments

[thierry bordaz]

On 06/17/2015 09:25 AM, Ludwig Krispenz wrote:
> Hi,
> thanks for review, see answers inline.
>
> On 06/16/2015 05:17 PM, thierry bordaz wrote:
>> On 06/16/2015 11:41 AM, Ludwig Krispenz wrote:
>>> this patch adresses issues in checking existing segments for one 
>>> directional segments and correctly handles the merging of segments, 
>>> so that all agreements will be removed when the merged segment is 
>>> deleted
>>>
>>>
>>
>> This is looking good to me with few comments
>>
>>   * in ipa_topo_cfg_replica_segment_find, if 'dir=0' or
>>     'dir=bidirectionnal' the reverse direction is bidirectionnal. Is
>>     it the expected result ?
>>
> yes. 0 does not exist as valid direct and if we are looking for 
> (A,B,both) this could als be expressed as (B,A,both). we do not really 
> look for a opposite direction of (A,B,dir) but for a segment 
> (B,A,revdir) which covers this segment.
>>
>>   *  in ipa_topo_check_segment_is_valid and
>>     ipa_topo_util_find_segment, may be hardening
>>     leftnode,rightnode,dir if they are NULL. (if the entry violate
>>     schema).
>>
> if we can arrive at a state where an entry violates the schema I think 
> we have more trouble, I want to avoid adding code for handling errors 
> which cannot exist.

Hi Ludwig,

thanks for your explanations. All of them makes sense and so for me the 
patch is valid.

I have a minor question about schema violation. When we add an entry, in 
preop we did not yet check the schema.
So ipa_topo_pre_add->ipa_topo_check_segment_is_valid may be called with 
an invalid segment entry where some attributes are missing (like 
ipaReplTopoSegmentDirection).

Also something that is not clear to.
I have a segment seg=ipa_topo_cfg_replica_segment_find(.., A, B, 
SEGMENT_RIGHT_LEFT, ..);. my understanding is that seg->right != 0 and 
seg->left == 0. is that correct ?

thanks
thierry
>>
>>   * ipa_topo_util_segm_dir if direction does not match any of the
>>     strings, it returns -1. 0 would be better if we decide to test
>>     bit mask.
>>
> yes, but in preop we check that only valid directions are added, so it 
> might be unnecesarry to handle it, but if you want I can change it.
>>
>>   * in ipa_topo_util_segment_update:810, ex_segm is a rigth_left
>>     segment. Why trying to call ipa_topo_cfg_agmt_dup with
>>     ex_segm->left in priority. Why not ex_segm->right first ?
>>
> no, we don't know if it is a right-left segment. we have 
> (A,B,left-right), the segment for the other direction could be 
> (A.B,right-left) or (B,A,left-right). All we know is that it is not 
> bidirectional, otherwise (A,B,left-right) would have been rejected in 
> the preop test. So there is one agmt, left or right and take the 
> existing one.
>>
>>  *
>>
>>
>>   * in ipa_topo_util_delete_segments_for_host, If segment
>>     localhost->delhost is bidirectional, how can it exists a reverse
>>     segment delhost->localhost ? I thought those segments have been
>>     merged ?
>>
> if it is bidirectional check_reverse is set to 0 and reveres is not 
> attempted
>>
>>
>> Thanks
>> thierry
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150617/22a4270d/attachment.htm>