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

[Tomas Babej]

On 06/29/2015 01:50 PM, thierry bordaz wrote:
> On 06/29/2015 12:47 PM, Martin Basti wrote:
>> On 17/06/15 11:05, Ludwig Krispenz wrote:
>>>
>>> On 06/17/2015 10:35 AM, thierry bordaz wrote:
>>>> 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).
>>> good point, in preop we cannot rely on schema been checked, need to
>>> add a check.
>>>>
>>>> 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 ?
>>> no :-) one directional segments are a bit confusing.  a replication
>>> agreement B-->A can be represented by a segment (A,B,right-left) or
>>> (B,A,left-right). when doing segment_find (A,B,right-left) we are
>>> looking if any segment covers this and teh result could be a segment
>>> (B,A,left right with seg->left !=0
>>>>
>>>> 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
>>>>>
>>>>
>>>
>>>
>>>
>> Hello, what is status of this patch?
>>
>> Also there are 2 whitespace errors.
>>
>> Ludwig's PATCH 15 depends on this patch, would be nice to have this
>> acked, to unblock review.
>>
>> Martin^2
>>
>> -- 
>> Martin Basti
> Hello,
> 
> The fix is good except some sanity checks that Ludwig will add with
> https://fedorahosted.org/freeipa/ticket/5088.
> 
> ACK
> 
> thanks
> thierry
> 
> 

Pushed to master: a86f2b3c624335a8f6bb211d52dc17b490b80d25