Subject: Comments on protocol draft

From: "Putzolu, David" <[email-masked]>
Date: Mon, 14 Nov 2005 08:17:35 -0800

My comments from review below. Most of them are editorial in nature and
have a single * next to them. The ones that are borderline or more have
** next to them. I will submit these as issues later today.

-David

---

Abstract:
* s/ForCES protocol is a message exchange protocol that is used
for/ForCES protocol is used for/

1. Introduction:
* s/aims to define an architectural framework/defines an architecture
framework/
* s/the term "ForCES protocol" refers only/"ForCES protocol" and
"protocol"
  as used in this document refers/
** "Information exchanged between FEs and CEs makes extensive use of
TLVs."
  Doesn't *all* control information use TLVs in ForCES?  
* s/The ForCES protocol is defined to have a common header/The ForCES
  Protocol defines a common header

2. Definitions
** The reference to the model seems normative.  Is this the case?
* s/(see below) to determine which FE to use./(see below) to determine 
  which FE(s) to use.
** The definition of LFB instance implies incorrectly that all LFB
instances
  are in the data path.  If anything, consider combining LFB Type and
  instance in a single definition because they are so closely related.
  The LFB definitions should also clearly call out that LFBs are a 
  functionally accurate abstraction of the FE's packet processing 
  capabilities, but not a hardware-accurate representation of the FE
  implementation.
* LFB Metadata - same comment, functionally but not implementation
  accurate.
** Consider merging all the definitions in this section rather than
  breaking them into separate subsections.  Since this document is
  coming before the model it can define the terms and the model can
  build upon them instead of vice versa.  
* The definition of ForCES Protocol Transport Mapping Layer has the
  phrase "how to achieve and implement reliability, multicast, ordering,
  etc."  Need to be very careful here as it is not in the ForCES 
  charter to create a new transport protocol.  Consider language such
  as "how to fulfill ForCES transport requirements using the
capabilities
  of the identified underlying transport protocols and mediums"
(although
  this language is ugly, hopefully someone can come up with something
  better).
** Does the doc every specify who sets priority levels?

3.1 Protocol Framework
* "The protocol element configuration is out of scope of the ForCE
protocol" - Do you mean CEM and FEM configuration here? Not clear.

3.1.2 The TML Layer
* s/layer is essentially responsible for transport of/layer transports/
** There is a "should" in here ("All ForCES Protocol Layer
implementations
  should be portable across all TMLs").  From the context, it feels like
  Is should be a MUST. Which is it?  If it is only a SHOULD, explain
why.

3.3.1.1.3. Transactional protocol
* There is an editorial note here that needs to be addressed. This feels
  borderline technical

3.3.1.1.5 continue-execute-on-failure
* The first sentence appears to be a fragment. Same comment applies to
  3.3.1.1.6.

3.3.1.1.6 execute-until-failure
* There is an editorial note here that borders on technical.

3.3.3 FE Object and FE protocol LFBs
* "speacilized"

3.3.4 Scaling by Concurrency
* The section title is confusing. How about just "Scalability"?
* s/mythical/hypothetical/

4. TML Requirements
* Shouldn't #3 also include preventing the link(s) to the CE being
  overwhelmed by traffic from FEs?
** #4 is very vague, I cannot tell what the requirement is
* Is #8 really a separate requirement from #7?
* s/priotity/priority/

5. Message encapsulation
* While it is probably safely assumed, I would like to see it explicitly
  called out that all fields are in network byte order.

5.1 Common Header
* s/donot/do not/
* s/Fe/FE/
** "64 bit pointer to a context block" Just as a comment, passing
pointers
  around is dangerous because a buggy FE (which munges a correlator) can
  cause a CE crash.
** For the ACK indicator, under the guideline of being conservative in
what
  we send, I would suggest the flag MUST be set to zero by the sender
  for all messages other than Config and heartbeat.  Also note that the
  language as currently written says the flag must be ignored for
heartbeat
  messages, which is probably incorrect. The language here also seems to
  contradict the language below that says, "Association Setup Message
always
  expects a response.".
* s/inidcate/indicate/

5.2. Type Length Value(TLV) Structuring
* Is the length in units of octets or in 32-bit words?

5.2.2. Scope of the T in TLV
* The sentence that starts with "This means that wherever" is confusing,
  can it be reworded?

6.1.1. Protocol BNF
* Reference is made to *0, but *0 appears to be illegal in this grammar?
* Isn't + more frequently used than 1* to indicate 1 or more
repetitions?
  I am not a BNF expert however.

6.1.1.1. Discussion on Grammar
* s/optionaly/optionally/

6.1.1.1.6. Operation TLV
* s/deleteing/deleting/

6.1.1.1.7. Result TLV
* Permission denied has no context. When might it be returned and why?

6.1.1.1.8. Data TLV
* Padding is mentioned. What is the padding, 0s or some other value?

6.2.1. FE Protocol LFB
* For clarity fully define the value. I.e. 0x0001
* The writable FE protocol attribute of ForCES protocol should call 
  out that the only legal values are those that are returned in the
  ForCES protocol version capability.
** I could not find where the CE heartbeat interval (must be shorter
  than the CE HDI) was stated, nor could I find a place to set it.
* s/associatin/association/

6.5.2. Config Response Message
* s/TThe/The/

8. High Availability Support
* Should the bottom All Msgs arrow in figure 33 have bidirectional
arrows?
  Same comment for figure 34.

Appendix C. ForCES Protocol LFB schema
* Has this been run through an XML validation tool?

Appendix F. changes between -03, -04, and -05
* Should there be RFC editor instructions to remove this section
  Upon publication?
* Same comment applies to Appendix G, H, I

Author's addresses appears twice

---

From: Jamal Hadi Salim <[email-masked]>
Date: Mon, 14 Nov 2005 11:56:27 -0500

David,

thanks for taking the time. I have added the tracker on the CC
and changed subject - hopefully it will catch this.

I noticed some of the things you had in your original email are missing
from the tracker entry.

Comments only on things that needed response; the rest are
mechanical/editorial and need fixing.

cheers,
jamal

>   Doesn't *all* control information use TLVs in ForCES?

We need to be more explicit:

We have two types of TLVs: 16 bit T, 16bit L and then
We have ILVs which use elementID for T ...

> ** The reference to the model seems normative.  Is this the case?

I dont think so - Joel? The dependency on the model at the moment rests
with the FEO; which we may end removing.

> instances are in the data path.  If anything, consider combining LFB 
> Type and instance in a single definition because they are so closely 
> related.

Good point. 

>   build upon them instead of vice versa.  

nod.

>   this language is ugly, hopefully someone can come up with something

make sense.

> ** Does the doc every specify who sets priority levels?

Done by both CE and FE.
The priority text needs some cleaning up. We dont for one specify which
priority is more important.

>   Is should be a MUST. Which is it?  If it is only a SHOULD, explain

Well, if we are going to define the service primitives then its a MUST;
else it is a SHOULD.

>   overwhelmed by traffic from FEs?

If they are overwhelmed then they are congested. What explicit text are
you thinking of.

> ** #4 is very vague, I cannot tell what the requirement is

The original idea was to allow for a mapping between a PL
multi/broadcast to an underlying transport multi/broadcast map.
If one was say to define a TML that does UDP multicast, they would have
to say how multicast PL ID X fits into a UDP port/IP mcast address.
 
> * Is #8 really a separate requirement from #7?

Looks like they need to be combined.

>   called out that all fields are in network byte order.

Sensible.

>   around is dangerous because a buggy FE (which munges a correlator) can

We should probably remove that text; it seems to be causing a lot of
confusion - it was meant as a mere example.

>   messages, which is probably incorrect. 

nod. that needs fixing

> The language here also seems to

32 bit words. Do you see any issues?

> * The sentence that starts with "This means that wherever" is confusing,

nod

>   I am not a BNF expert however.

The grammar definition in RFC 2234 has no concept of + rather 1*.
Joel, comment?

> * Permission denied has no context. When might it be returned and why?

I think once we have IANA definitions in place, the text for all the
returned codes will be explicit.

> * Padding is mentioned. What is the padding, 0s or some other value?

We should be explicit; zeros.

> * For clarity fully define the value. I.e. 0x0001

IANA text?

>   than the CE HDI) was stated, nor could I find a place to set it.

Yep, we need to be explicit

>   Same comment for figure 34.

yep

> * Has this been run through an XML validation tool?

Tracker issue opened on the xml

cheers,
jamal

---

From: "Joel M. Halpern" <[email-masked]>
Date: Mon, 14 Nov 2005 16:59:06 -0500

Actually, opinions seem to differ.
I consider that the protocol normatively depends upon the model in several 
regards.
The data paths are defined by model constructs.  It is not possible to talk 
about what is identified without that.
I think that there are other similar issues.
One way to see this is that there are quite a few changes I could imagine 
the WG making to the model which would require core protocol changes.

My goal is to get the revisions to the model draft done so that having the 
normative dependency will not affect RFC publication.

Yours,
Joel M. Halpern