Subject: [shim6] review draft version 12

From: Sébastien Barré <[email-masked]>
Date: Thu, 14 Jan 2010 12:50:09 +0100

Hi Miika, all,

I finally found time to review the API draft.
Sorry for reviewing so late ! Note the comments hereafter are about 
draft version 12, and are a mix of editorial fixes and quite important 
high level disagreements. I hope it is clear :-) .

*section 1, §4: ...means a singed integer of... -> a signed integer
*Section 4, 3rd bullet, 2nd star: "Besides, a receipt of an ICMP error 
message could be a clue for the application to detect problems"
I am not sure that this is a good idea. IMO, this is equivalent to say 
that when a stack receives an ICMP packet too big, the application 
should listen to it and inform TCP that it should reduce its MSS. This 
is not what happens: the stack intercepts the ICMP and propagates the 
information about new MTU to the interested layers. Based on this 
existing example, I tend to think that, while an application *can* 
listen to ICMP error messages for any reason, it should *not* use it to 
inform the shim layer, since that layer has already got the information. 
To summarize, the only useful feedback from app to shim IMO is 
information that only the app can get (e.g. repeatingly loosing app 
messages).
*Section 4, 8th bullet: "A deferred context setup means that the 
initiation of communication should not be blocked to wait for completion 
of the context establishment." : I do not understand why such an option 
would be useful. I mean, we have two possible situations. First is that 
we technically *must* wait for completion because routing is not 
possible before the completion of the context establishment. This 
happens if we have non-routable EIDs. Second situation is when we 
*could* wait for completion, but not mandatorily. This happens when the 
EIDs are routable.
In the first situation, the option has no effect anyway, since there is 
no choice. In the second situation, why would I prefer to wait for the 
context to be established, when nothing stops me from immediately 
speaking with my peer ? OTOH, I see as useful another definition of this 
option, that would be something like "Applications should be able to 
differ context establishment. A deferred context setup means that the 
application does not want to start the context establishment immediately 
when starting the communication." And this requirement is fulfilled with 
the SHIM_DONTSHIM option. The app can first set the option, and then 
unset it as soon as it wants the context to be established. If we do it 
that way, we can specify that by default the Shim applies any internal 
heuristic to decide when to start context establishment, but 
SHIM_DONTSHIM overrides that behaviour, so that when it is set we dont 
negotiate, and when it is cleared again, we negotiate immediately 
without waiting for the internal heuristic. Makes sense ?
*Table 1 - SHIM_ASSOCIATED: whether if the socket -> whether the socket
*legend of table 1: *1 and *2: why to separate this in two definitions, 
is it not the same ? I would merge this into one reference.
*Section 5.4 and 7.2 : the name pe_keepaliveto is somewhat misleading. I 
would call it rather pe_sendto, since it is actually the send timeout 
option that you are defining. The SEND timer the timer that triggers the 
exploration, as you are defining. The keepalive timer, OTOH, is the 
timer the decides for how long we send keepalives. What the REAP RFC 
tells about that, is that when a shim6 peer decides to change the send 
timeout, it must inform the peer about that, copying the *send* timeout 
value in a Shim6 option called the *keepalive* option (rfc5534, section 
5.3). That may cause some confusion indeed, but makes sense since the 
send timeout of one peer is the keepalive timeout of the other peer. At 
least, this is what I have understood from the RFC, and what is 
implemented in LinShim6.
This comment has implications on other parts of the draft:
*Section 5.13, SHIM_APP_TIMEOUT: now you are speaking about the send 
timeout, but you give it the definition that RFC5534 gives to the 
keepalive timeout.
 Anyway,I believe that we can remove this option altogether. Reason is 
that an application *cannot* have control on the keepalive timeout. As 
indicated above, *you* decide about your send timeout (that triggers the 
path exploration), and *your peer* decides about your keepalive timeout. 
There is no reason why you would decide yourself about the keepalive 
timeout since it *must* be tuned according to the send timeout of the peer.
BTW, about the following paragraph from section 5.13:"If the timeout 
value specified is longer than the Send Timeout configured in the REAP 
component, the REAP Keepalive message should be suppressed." ->Why would 
there be any reason to suppress that message ? Stopping to send that 
message almost completely disables REAP.
*Section 5.14, SHIM_DEFERRED_CONTEXT_SETUP: Again, I would remove that 
option altogether. See above comment about deferred context setup. BTW, 
I do not follow why to "defer" a context establishment would mean "do it 
immediately" as specified in the draft. "To defer", means postpone to 
later, which is exactly the opposite.
*Section 5.5 and 5.6 : SHIM_LOC_LOCAL_PREF and SHIM_LOC_PEER_PREF: As 
defined, these options are duplicate of SHIM_LOC_LOCAL_SEND and 
SHIM_LOC_PEER_SEND. Thus I would remove two of them, and keep preferably 
the SHIM_LOC_*_SEND version, since the SHIM_LOC_*_PREF version is 
confusing due to section 5.15.3 of rfc 5533 that defines differently the 
locator preferences.
*Section 5.5, §4: shall verify requested locator before it updating -> 
shall verify *the* requested locator before it *updates*
*Section 5.5, code snippet: memcpy(lc.lc_addr,... -> memcpy(&lc.lc_addr,...
*Section 5.6, §4: it updating -> it updates
*Section 5.9, §7: when invalid locator -> when an invalid locator
*Section 5.10,last §: is as the same as -> is the same as
*Section 5.11,§4:an array of locator list -> an array of locator structures
*Section 5.11,§6:the validation of the specified locator -> the 
validation of *any of* the specified *locators*
*Section 5.11: It would be great to specify what happens when we provide 
too small a buffer. Because this is the situation that could well 
happen. Do we return ETOOMANYLOCATORS ? I guess the definition you give 
is for the setsockopt, but it could serve as well for the getsockopt, 
when the kernel has more locators to provide than allowed by the space 
that the app has allocated.
*Section 5.12, §4: pointed by optval argument -> pointed to by the 
optval argument (I think at least... If this is true, you should do a 
replace all, this appears at several places in the draft).
*Section 5.12,§6: the validation of the specified locator -> the 
validation of *any of* the specified *locators*
*Section 6.5: A care is needed -> Care is needed (I think)
*Section 7.1: The list of options that use the shim_locator structure 
does not mention SHIM_LOC_*_SEND. I guess it should.
*Section 8: "Note that the shim sub-layer should be able to backtrack 
the socket from
   which the user data was originated." : Probably it is true, but  is 
it not dependent on the implementation ? I guess we could design an 
implementation where no backtrack is necessary, and all requirements are 
satisfied.
*Section 9, §2, again, confusion between SHIM_LOC_*_SEND and SHIM_LOC_*_PREF
*Section 10.1,§2: "When a forked context is torn down, the SHIM6 
implementation should notify the peer about the deletion of forked 
context." : This is contrary to rfc5533, that specifies no message to 
notify the peer of context deletion. Instead, it defines all the 
necessary stuff to live well with the fact that one of the peers can 
unilaterally decide that it drops a context, without telling anything.
*high level comment: An interest conflict may happen between one peer 
and the other. Imagine we have A and B that communicate. A learns the 
locators from B: B1,B2,B3. The app prefers receiving segments through 
B2, so it sets  a high preference value to that locator. Shim6 will 
communicate this to the peer through a locator preference option. But if 
the peer has itself set a SHIM_LOC_LOCAL_SEND option, we have a 
conflict. Who wins ? I guess the best it that when local and remote end 
are in conflict, the winner is the local end. Right ? So if everyone 
agree with that detail, maybe it is worth mentioning it in the API as well.

regards, and sorry again for having read the draft so late !

Sébastien.

-- 
Sébastien Barré
Researcher,
CSE department, UCLouvain, Belgium
http://inl.info.ucl.ac.be/sbarre

---

From: Shinta Sugimoto <[email-masked]>
Date: Sun, 17 Jan 2010 22:59:32 +0900

Dear Sébastien,

Thank you very much for your comments.  This is very helpful indeed. 
Please find my comments inline.

 > *section 1, §4: ...means a singed integer of... -> a signed integer

Ok.

 > information that only the app can get (e.g. repeatingly loosing app

Ok, I see.  You are right that application cannot know the receipt of
ICMP message unless it opens up another raw socket and monitors receipt
of ICMP messages.  Let us remove the sentence that you pointed out.

 > speaking with my peer ?

I see your point and I agree.  I realized that the description made in
section 4 about deferred context setup and the intention of the
SHIM_DEFERRED_CONTEXT_SETUP are not aligned each other.  As you stated
above, there is no point in providing application an ability to enable
deferred context setup.  Instead, I think it makes sense to provide
application an ability to check if deferred context setup is possible or
not.  Application may want to know this before sending data to the peer.
Suppose application has no idea what type of shim sub-layer (i.e., HIP
or SHIM6) is available on the system.  Application may have no idea
about the type (whether it is routable or non-routable) of EIDs used for
communication.  Then, such an application can simply know if the
communication is to be blocked or not before the context is in place.
Actually, SHIM_DEFERRED_CONTEXT_SETUP is intended to provide this
feature as it is mentioned in page 12 ("parameter which indicates
whether deferred context setup is supported or not.").  So, do you think
it makes sense to rephrase the 8th bullet of Section 4 as below?

Applications should be able to know if the shim-sublayer supports
deferred context setup or not.

 > OTOH, I see as useful another definition of this

Yes, I understand.  I think there is no need to modify texts in the
document in this respect, though.

 > *Table 1 - SHIM_ASSOCIATED: whether if the socket -> whether the socket

Ok, thanks.

 > is it not the same ? I would merge this into one reference.

You are right.  Let us merge *1 and *2.

 > This comment has implications on other parts of the draft:

Ok, I understand.  I think I misinterpreted the meaning of keepalive
timeout and send timeout.

 > timeout, but you give it the definition that RFC5534 gives to the

Ok.

 > timeout since it *must* be tuned according to the send timeout of the
peer.

Ok.  I understand and agree that we should not specify any socket option 
to specify the Keepalive Timout value.  Instead, we should provide 
socket option to specify the Send Timeout value.

 > message almost completely disables REAP.

No.  You are right.

In summary, let us update the draft as follows with regard to the REAP 
related features:

- 5.13: rewrite texts by providing definition of a socket option which 
allows application to specify Send Timeout value.
- 7.2: s/pe_keepaliveto/pe_sendto

does this sound reasonable?

 > later, which is exactly the opposite.

As I stated above, the descriptions given in Section 5.14 is not aligned
with the intended purpose of the socket option.  Sorry for the confusion
and let us update the texts accordingly.  As I mentioned above, the
intended purpose of SHIM_DEFERRED_CONTEXT_SETUP is to allow application
to know if deferred context setup is supported by the shim sub-layer or not.

 > confusing due to section 5.15.3 of rfc 5533 that defines differently the

let me come back to you on this once I finish revisiting the RFC 5533 
and check the concept of the locator preference there.

 > *Section 5.5, code snippet: memcpy(lc.lc_addr,... ->
memcpy(&lc.lc_addr,...

Ok.

 > *Section 5.6, §4: it updating -> it updates

Ok.

 > *Section 5.9, §7: when invalid locator -> when an invalid locator

Ok.

 > *Section 5.10,last §: is as the same as -> is the same as

Ok.

 > *Section 5.11,§4:an array of locator list -> an array of locator
structures

Ok.

 > validation of *any of* the specified *locators*

Ok.

 > that the app has allocated.

I see.  Yes, I think the kernel should return ETOOMANYLOCATORS when the
available locators does not fit in the buffer given by the application.

 > replace all, this appears at several places in the draft).

Ok.

 > validation of *any of* the specified *locators*

Ok.

 > *Section 6.5: A care is needed -> Care is needed (I think)

Ok.

 > does not mention SHIM_LOC_*_SEND. I guess it should.

Right.

 >   which the user data was originated." : Probably it is true, but  is it
 > not dependent on the implementation ? I guess we could design an

Yes, this is very much implementation specific.  But I am not sure if I
get your last sentence.  What implementation are you referring to?  A
shim sub-layer implementation? or the API specification and implementation?

 > *Section 9, §2, again, confusion between SHIM_LOC_*_SEND and

Ditto. (let me responde in separate email on this issue)

 > unilaterally decide that it drops a context, without telling anything.

Ok, I see.  Will just remove the last sentence in the paragraph.

 > agree with that detail, maybe it is worth mentioning it in the API as
well.

I see.  I agree that the owner of the locator should win in such a
conflicting case.  And I agree that it should be stated in the API
document if there is objection from the WG members.  Please let us know
if you have any opinions > WG members

Thanks again for your thorough review!

Regards,
Shinta

---

From: Shinta Sugimoto <[email-masked]>
Date: Wed, 20 Jan 2010 06:29:52 +0900

Sébastien,

Let me address my view on SHIM_LOC_*_PREF and SHIM_LOC_*_SEND.

The two have different meaning.  SHIM_LOC_*_SEND allows application to 
specify source or destination locator, whereas SHIM_LOC_*_PREF allows 
application to specify preference of a given locator.  Yes, they are 
similar in the sense that both allow application to make selection of 
source or destination locator in some form.  However, I prefer to keep 
SHIM_LOC_*_PREF because  I think the concept of handling preference of 
locators is useful, especially in the cases where are multiple choices. 
  But I admit that the current definition of SHIM_LOC_*_PREF in the 
document is not aligned with what is defined in RFC 5533, which is 
confusing.  We should fix this.

as stated in RFC 5533, preference of locator shall be handled in the 
same way as RFC 2782 (DNS SRV).  That is, a preference is represented by 
a pair of priority and weight values.  So, I would suggest to keep 
SHIM_LOC_*_PREF by applying the following changes to the document:

- update definition of locator information structure (shim_locator{}) in 
section 7.1 (define a 16-bit unsigned integer parameter named lc_weight 
to store weight value ranging from 0-65535)
- update texts in section 5.5 and 5.6 to make the definition of 
preference aligned with RFC 5533.
- provide explanations on how ths shim sub-layer would react in the case 
of conflicting situations.  There are at least two situations that we 
should think of.  I think Section 10 (Operational Considerations) would 
be the right place to have these texts.

1) when the application specifies SHIM_LOC_*_SEND specifying a different 
source or destination locator which does not have the highest 
priority/weight specified by the SHIM_LOC_*_PREF, the request made by 
SHIM_LOC_*_SEND must overwrite the prefenrece specified by SHIM_LOC_*_PREF.

2) when the peer provides preference of the locators, that must 
overwrite the preference specified by SHIM_LOC_PEER_SEND or 
SHIM_LOC_PEER_PREF.

Regards,
Shinta

---

From: Sébastien Barré <[email-masked]>
Date: Wed, 20 Jan 2010 15:11:39 +0100

Shinta,

Shinta Sugimoto a écrit :
> locators is useful, especially in the cases where are multiple 
> choices.  But I admit that the current definition of SHIM_LOC_*_PREF 
> in the document is not aligned with what is defined in RFC 5533, which 
> is confusing.  We should fix this.
>
> same way as RFC 2782 (DNS SRV).  That is, a preference is represented 
> by a pair of priority and weight values.  So, I would suggest to keep 
> - update definition of locator information structure (shim_locator{}) 
> in section 7.1 (define a 16-bit unsigned integer parameter named 
> lc_weight to store weight value ranging from 0-65535)
makes sense.
> preference aligned with RFC 5533.
ok
> - provide explanations on how ths shim sub-layer would react in the 
> case of conflicting situations.  There are at least two situations 
> that we should think of.  I think Section 10 (Operational 
> Considerations) would be the right place to have these texts.
>
> 1) when the application specifies SHIM_LOC_*_SEND specifying a 
> different source or destination locator which does not have the 
> highest priority/weight specified by the SHIM_LOC_*_PREF, the request 
> made by SHIM_LOC_*_SEND must overwrite the prefenrece specified by 
> SHIM_LOC_*_PREF.
ok
> overwrite the preference specified by SHIM_LOC_PEER_SEND or 
Well, regarding that point: Are you sure we should not do it the other 
way around ? I mean, imagine a server, that for some applications, does 
not want to use one of its paths for security reasons. If now I want to 
force the use of the normally forbidden path, I can just send a locator 
preference option, and this will override the local preference of the 
server. This is why I think we should instead always give the priority 
to local choices. And if locally two paths are considered equal, only 
then can they be classified according to peer preferences.

Also, How does an application mention that its setting of locator 
preference is inbound or outbound ? If it is outbound, the shim must 
just reorder its locator list. If it is inbound, the shim must send 
locator preference option to the peer stating the new value of 
preferences. Or is it considered always true that the setting of a 
locator preference is both inbound and outbound ? It might be indeed, 
but anyway maybe it is good in that case to explicitly mention that the 
preference setting is bidirectional, and thus the shim should notify the 
peer about the new preference.

Did i miss something ?

regards,

Sébastien.
>> SHIM_LOC_PEER_SEND. Thus I would remove two of them, and keep 
>> preferably the SHIM_LOC_*_SEND version, since the SHIM_LOC_*_PREF 
>> version is confusing due to section 5.15.3 of rfc 5533 that defines 
>> differently the locator preferences.
>
>

---

From: Shinta Sugimoto <[email-masked]>
Date: Fri, 22 Jan 2010 00:09:01 +0900

Hi Sébastien,

> then can they be classified according to peer preferences.

You are right.  My intention was the other way around, as the choice of 
destination locator must be made by the sender (as far as the selected 
destination locator appears in the locator list).  Sorry for the 
confusion.  So, the proposal is to add the following texts:

2) preference of the destination locator specified by SHIM_LOC_PEER_SEND 
or SHIM_LOC_PEER_PREF must supersede the preference specified by the peer.

> preference is inbound or outbound ? If it is outbound, the shim must

SHIM_LOC_{PEER,LOCAL}_PREF is only for outbound.  In my understanding, 
it does not make sense to allow application to set preference for 
inbound packet processing (it's up to the peer).  As far as the locators 
(source and destination) appear in the locator list that the receiving 
peer has, it needs to receive the packet.  And the Shim6 spec does not 
specify any means for a peer to inform the preference of the destination 
locator of inbound traffic (and I think it makes sense to not define 
such a means).

The name SHIM_LOC_{PEER,LOCAL}_PREF does not explicitly tell whether the 
preference is for inbound or outbound (or both), and there is no 
explanation in each section either.  So, let me add texts to section 5.5 
(SHIM_LOC_LOCAL_PREF) and section 5.6 (SHIM_LOC_PEER_PREF) stating that 
the preference set by application is only effective for outbound traffic.

> peer about the new preference.

Please let me know if my explanation above makes sense.

Regards,
Shinta