Subject: Re: issue 63 Was(Re: Comments on protocol draft

From: "Putzolu, David" <[email-masked]>
Date: Mon, 14 Nov 2005 12:29:47 -0800

Thanks Jamal - I have taken those of my comments that required further
changes based on what you wrote below and entered them in the issue
tracker as new issues or updates to old issues that they where related
to.  The tracker now has all the issues/comments I made that remain
open.

Regards,
David

-----Original Message-----
From: Forwarding and Control Element Separation
[mailto:[email-masked]] On Behalf Of Jamal Hadi Salim
Sent: Monday, November 14, 2005 8:56 AM
To: [email-masked]
Subject: [FORCES] issue 63 Was(Re: Comments on protocol draft

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

On Mon, 2005-14-11 at 08:17 -0800, Putzolu, David wrote:

> ** "Information exchanged between FEs and CEs makes extensive use of
> TLVs."
>   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 ...

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

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

Good point. 

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

nod.

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

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.

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

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

> 4. TML Requirements
> * Shouldn't #3 also include preventing the link(s) to the CE being
>   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.

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

Sensible.

> 5.1 Common Header

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

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

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

nod. that needs fixing

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

32 bit words. Do you see any issues?

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

nod

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

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

> 6.1.1.1.7. Result TLV
> * 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.

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

We should be explicit; zeros.

> 6.2.1. FE Protocol LFB
> * For clarity fully define the value. I.e. 0x0001

IANA text?

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

Yep, we need to be explicit

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

yep

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

Tracker issue opened on the xml

cheers,
jamal