<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 06/17/2015 10:35 AM, thierry bordaz
wrote:<br>
</div>
<blockquote cite="mid:5581316A.1020608@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 06/17/2015 09:25 AM, Ludwig
Krispenz wrote:<br>
</div>
<blockquote cite="mid:558120D1.2010700@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi,<br>
thanks for review, see answers inline.<br>
<br>
<div class="moz-cite-prefix">On 06/16/2015 05:17 PM, thierry
bordaz wrote:<br>
</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:<br>
</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 <br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
<br>
<font face="Times New Roman, Times, serif">This is looking
good to me with few comments<br>
<br>
</font>
<ul>
<li><font face="Times New Roman, Times, serif">in
ipa_topo_cfg_replica_segment_find, if 'dir=0' or
'dir=bidirectionnal' the reverse direction is
bidirectionnal. Is it the expected result ?</font></li>
</ul>
</blockquote>
<font face="Times New Roman, Times, serif">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.</font><br>
<blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite">
<ul>
<li><font face="Times New Roman, Times, serif"> 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).</font></li>
</ul>
</blockquote>
<font face="Times New Roman, Times, serif">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.</font><br>
</blockquote>
<br>
<font face="Times New Roman, Times, serif">Hi Ludwig,<br>
<br>
thanks for your explanations. All of them makes sense and so for
me the patch is valid.<br>
<br>
I have a minor question about schema violation. When we add an
entry, in preop we did not yet check the schema.<br>
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).<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">good point, in preop we
cannot rely on schema been checked, need to add a check.</font><br>
<blockquote cite="mid:5581316A.1020608@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
Also something that is not clear to.<br>
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 ?<br>
</font></blockquote>
<font face="Times New Roman, Times, serif">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<br>
(B,A,left right with seg->left !=0<br>
</font>
<blockquote cite="mid:5581316A.1020608@redhat.com" type="cite"><font
face="Times New Roman, Times, serif"> <br>
thanks<br>
thierry<br>
</font>
<blockquote cite="mid:558120D1.2010700@redhat.com" type="cite">
<blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite">
<ul>
<li><font face="Times New Roman, Times, serif">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.</font></li>
</ul>
</blockquote>
<font face="Times New Roman, Times, serif">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.</font><br>
<blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite">
<ul>
<li><font face="Times New Roman, Times, serif">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 ? <br>
</font></li>
</ul>
</blockquote>
<font face="Times New Roman, Times, serif">no, we don't know if
it is a right-left segment. </font>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.<br>
<blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite">
<ul>
<li><font face="Times New Roman, Times, serif"> </font><br>
</li>
<li><font face="Times New Roman, Times, serif">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 ?</font></li>
</ul>
</blockquote>
<font face="Times New Roman, Times, serif">if it is
bidirectional check_reverse is set to 0 and reveres is not
attempted</font><br>
<blockquote cite="mid:55803DFF.7040805@redhat.com" type="cite">
<ul>
</ul>
<font face="Times New Roman, Times, serif"><br>
Thanks<br>
thierry </font><br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>