<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi, thanks for review, see answers inline. <div class="moz-cite-prefix">On 06/16/2015 05:17 PM, thierry bordaz wrote: </div> <blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite"> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 06/16/2015 11:41 AM, Ludwig Krispenz wrote: </div> <blockquote cite="mid:557FEF38.9000700@redhat.com" type="cite">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 <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> This is looking good to me with few comments <ul> <li>in ipa_topo_cfg_replica_segment_find, if 'dir=0' or 'dir=bidirectionnal' the reverse direction is bidirectionnal. Is it the expected result ?</li> </ul> </blockquote> 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. <blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite"> <ul> <li> 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).</li> </ul> </blockquote> 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. <blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite"> <ul> <li>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.</li> </ul> </blockquote> 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. <blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite"> <ul> <li>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 ? </li> </ul> </blockquote> 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. <blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite"> <ul> <li> </li> <li>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 ?</li> </ul> </blockquote> if it is bidirectional check_reverse is set to 0 and reveres is not attempted <blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite"> <ul> </ul> Thanks thierry </blockquote> </body> </html>