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@xxxxxxxx https://www.ietf.org/mailman/listinfo/ietf