[shim6] review draft version 12

[Sébastien Barré <[e-mail]>]

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