[Freeipa-devel] [PATCH 0014] correct handling of one directional segments
Martin Basti
mbasti at redhat.com
Mon Jun 29 10:47:40 UTC 2015
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150629/f5c67869/attachment.htm>
More information about the Freeipa-devel
mailing list