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

[thierry bordaz]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150629/8d30c654/attachment.htm>