I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
_http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html_).
Please resolve these comments along with any other Last Call comments
you may receive.
Document: draft-ietf-forces-model-14.txt
Reviewer: Elwyn Davies
Review Date: 5 Setember 2008
IETF LC End Date: 8 September 2008
IESG Telechat date: (if known) -
Summary: Nearly ready for IESG. Generally this is a very well
constructed and written document dealing with a very complex problem.
There are quite a number of minor points that ought to be addressed
(although not vast fpr a 132 page document) plus a (largish) number of
editorial nits (of which many are related to highly inconsistent choice
of what to use for the singular form of metadata - metadata or metadatum
- in many cases both are used in the same sentence!) The other
consideration that needs to be addressed is the use of normative
language in s4 - this is discussed below.
Caveat: I haven't done detailed checks of the XML schema and the long
bits of XML.
Comments:
Minor Issues:
=============
s2.3: States that the protocol can be used to discover the Inter-FE
topology: is what is meant here? or the intra-FE tolopology? Maybe be a
forward reference to s5.3.4 might clarify this if Inter-FE is right?
s3.1.1.3, para 2: 'For example, when
certain features of an LFB class are optional, the CE MUST be able to
determine whether those optional features are supported by a given
LFB instance.'
I have a vague feeling that this conflicts with the ‘suck it and see’
approach of coarse capabilities from the previous section. Also putting
a MUST into an example is unsatisfactory. It isn’t necessary here as
this is just the concepts section.
s3.2, para 8 (p16): 'In addition, the CE MUST understand where and what
type of header
modifications (e.g., tunnel header append or strip) are performed by
the FEs. Further, the CE MUST verify that the various LFBs along a
datapath within an FE are compatible to link together.'
I don't believe these are normative MUSTs. There is nothing the model
can do to enforce them.
s3.2.1, para 8 (p19): '...based on a FRAMETYPE metadata (N=2), or to fork
into color specific paths after metering using the COLOR metadata
(red, yellow, green; N=3), etc.'
The FRAMETYPE and COLOR hargon may be confusing to a novice reader and
are not strictly necessary.
s3.2.5: Event target terminology: It may be a bit late to change but the
term 'target' seems to convey the wrong idea. The 'target' actually is
the source of the event. Maybe 'anchor' might be better (taking our cue
from xml2rfc).
s4: General: The use of 'MUST' in many places but almost always 'may' is
IMO confusing. I think the problem is that the normative language is
(AFAICS) used to constrain the semantics of the XML schema - it isn't
about protocol behaviour. Now this is a reasonable use for this sort of
language but I think that at least some of the 'may's should also be
MAY. One way I thought about this could be to think of the section as
specifying the bahaviour of a tool that checks the semantics of the XML.
Stating this explicitly could then make it much clearer whether the
right language is being used. Please consider how to improve this.
s4.3: 'The load element MUST contain the label of the library document to be
included and may contain a URL to specify where the library can be
retrieved.'
Does anything need to be said about where it would come from if the
location os missing?
s4.5: The format of 'float16' needs to be defined. There probably needs
to be a reference to the right IEEE document that defines float32 and
float64.
s4.5: 'The boolean data type is defined here because it is
so common, even though it can be built by sub-ranging the uchar data
type.'
Two issues here: 1) sub-ranging hasn’t been talked about yet; 2) this
puts doubt into peoples’ minds about the size of the represenation.. is
a boolean 8 bits or 1 bit? Does this matter?
ss4.5, s4.5.6, para 1, s4.7.6.4, s8.3, 8.4 (and maybe elsewhere): These
sections refer to message names from the ForCES protocol. This is not a
good idea since it creates a bidirectional dependency between the docs.
generic terminology could be used rather than specific message names. I
am not entirely sure how to fix s8, but the current situation is not
totally desirable.
s4.5, last para: 'The predefined type alias is somewhere between the
atomic and
compound data types. It behaves like a structure, one component of
which has special behavior.' BUT *what is* an alias?? We should be told!
s4.5.3: <arry> Elelemt definition: Because of the talk about key fields
and use of structure components, it might be cleaner to place this after
structs and unions.
s4.5.3, para 2: 'The latter should be handled by capability components
of LFB classes, and should never be included in a data type array
which is regarded as of unlimited size' In the last part of this
sentence, it should be clarified if you are talking about 'maxlength'
attribute.
s4.5.3, para 4: 'The result of this construct MUST always be a compound
type, even if
the array has a fixed size of 1.' This is an inherent characteristic of
the type rather than something that is an implementation decision. Maybe
‘is necessarily’. Presumably the point is that it can’t be used fror
deriving <atomuc> elements. Same applies to struct (s4.5.4, para 5) and
union (s4.5.5, para 2).
s4.5.3, para 8: 'o In any instance of the array, each declared key MUST
be unique
within that instance. No two components of an array may have the
same values on all the fields which make up a key.' The second part of
this statement would be better rewritten in normative language.
s4.5.3, next to last para: 'When the current context is an array, (e.g.,
when declaring a key for
an array whose content is an array) then the only valid value for the
field identifier is an explicit number.' Should you say anythng about an
index into a variable aize array where the index might be out of bounds
for some instances? (nasty corner case!)
s4.5.4, para 3: 'The <component> element MUST include a componentID
attribute. This
provides the numeric ID for this component, for use by the protocol.'
Presumably it is NOT required that numeric IDs are in sequence and start
from 1.. the id’s and the definition order does not say anything about
the order of compnents in the model… or do they have any knock-on impact
into the protocol?? There is (or maybe) a distinction between C, where
order is HIGHLY significamt, and the model. This also has an effect on
extensions in augmentations.
s4.5.4, para 4: 'This is indicated by including the optional derivedFrom
attribute in
the struct declaration before the definition of the augmenting or
replacing components': A forward ref to 4.5.7 would avoid the ‘how?’
question.
s4.5.6, para 2: 'The target of a component declared by an <alias> element is
determined by the component’s properties.': It would be good to clarify
that these are properties of the component in the alias (If I have
understood this right.. I was unsure I understood correctly).
s4.5.7: Augmentations allow additions to structures: Does this
explicitly exclude unions? If so it woud be a good idea to make it
clear. If not they need to be talked about.
s4.5.7, para 1: 'The type of an
existing component can be replaced in the definition of an augmenting
structure, but only with an augmentation derived from the current
type of the existing component.' This would better redrafted in proper
normative language.
s4.5.7, last para: 'Component names and component IDs for new components
within the augmentation must not be the same as those in the
structure type being augmented.':
Make it clear if there are (or are not) any constraints on the numeric
IDs. I think the intention is that they are arbitrary and not
necessarily in sequence either withn themselves or with the ones in the
base class but I may be wrong!
s4.7.1, last para: 'It is assumed that a derived class is backwards
compatible with its
base class.'
What does this say about the ports and capabilities? Backwards
compatibility was previously discussed wholly in terms of the components
of LFB classes.
s4.7.6, para 2: 'The <events> element contains 0 or more <event>
elements': Why 0 or more? <events> is optional and all the other similar
cases require 1 or more if present.
s4.7.6.1: See earlier discussion of the use of 'target' for events.
s4.8, last para and s4.8.6: 'PL protocol': Is this the ForCES protocol
in some earlier teminology?
s4.6.5, dataType spec: <component componentID="3">
<name>threshold</name>
<synopsis> comparison value for level crossing events
</synopsis>
<optional/>
<typeRef>uint32</typeRef>
</component>
<component componentID="4">
<name>eventHysteresis</name>
<synopsis> region to suppress event recurrence notices
</synopsis>
<optional/>
<typeRef>uint32</typeRef>
</component>
The implication of this appears to be that thresholds must be unsigned
integer values and the hysteresis is then naturally an integer. Is this
intended? Is there any reason why a threshold couldn’t be defined on a
floating point number (apart form the high computational requirements?
s9.2: maybe should reiterate that Class Identifiers are 32 bit quantities.
Editorial:
==========
(I have sent a marked up Microsoft Word file with the minor editorial
issues to the editors and they are not repeated here.)
s1,LFB Model definition: May require a definition of the term 'frame'.
s1, FE Topology definition: It is necessary to expand the abbrevations
imported from RFC 3654 - in practice I think there are not many - the
one here is NE.
Figure 2: Note meanings of P and M in text.
s3.2.4.2, last para: 'vice versa' is superfluous - either the metadata
are the same or not.. which one was originally defined first is only on
the mind of the designer.
[S4.7.1: Enlighten me please! Is a 'vifid' like a Triffid with video?]
s4.8.5, para 1: 'The hysteresis field is used to suppress generation of
notifications for oscillations around a condition value, and **is
described in the text for events**': I don’t think it is.. there is
brief reference in s3. Alternative: reference s4.8.5.2 below.
s4.8.5.3, last para: 'it is
reset to 0. In this conceptual implementation the reset behavior
when a notification is generated can be thought of as setting the
counter to 1.': The dual use of 'reset' in this is slightly confusing.
s8.4: Uses the term 'variance property' whan talking about the
capability for 'hysteresis' - I don't think this term is used elsewhere.
_______________________________________________
Ietf mailing list
Ietf(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/ietf