Re: [shim6] review draft version 12

[Shinta Sugimoto <[e-mail]>]

Dear Sébastien,

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

Sébastien Barré wrote:
 > 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

Ok.

 > *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).

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.

 > *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 ?

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
 > 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 ?

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.

 > *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.

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

 > *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:

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

 > *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.

Ok.

 > 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.

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.

 > 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.

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?

 > *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.

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.

 > *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.

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, §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,...

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.

 > *Section 5.11,§6:the validation of the specified locator -> the
 > validation of *any of* the specified *locators*

Ok.

 > *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.

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

 > *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).

Ok.

 > *Section 5.12,§6: the validation of the specified locator -> the
 > validation of *any of* the specified *locators*

Ok.

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

Ok.

 > *Section 7.1: The list of options that use the shim_locator structure
 > does not mention SHIM_LOC_*_SEND. I guess it should.

Right.

 > *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.

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
 > SHIM_LOC_*_PREF

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

 > *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.

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

 > *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.

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