[Freeipa-devel] [PATCH 0014] correct handling of one directional segments
Ludwig Krispenz
lkrispen at redhat.com
Wed Jun 17 07:25:05 UTC 2015
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.
>
> * 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/e87f78fa/attachment.htm>
More information about the Freeipa-devel
mailing list