[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

snmpconf review of pm-mib



Title: review of pm-mib

Hi,

I have reviewed the pm mib through section 9.3. More to come.

dbh

 

review of draft-ietf-snmpconf-pm-09
Part 1
David Harrington

I did a review of -08- by mistake. There are few differences, but if I cite a section number and it doesn't have the text mentioned, it might be a versioning issue.

A. Nits based on http://www.ietf.org/ietf/1id-guidelines.txt

1) the format of the first page header doesn't follow rfc2223, and seems different than most.
2) the initial paragraph "This is an Internet-draft..." differs
 from the template. I think the template changed slightly.
3) the table of contents should be on page 2.
4) In the running footer, why not list the authors' names rather than
"various authors"?
5) in section four, there is discussion of context and execution context, and it is a bit confusing. I think it would help to clearly label SNMP contexts as SNMP contexts, or to use contextName to disambiguate this.
6) section 5.1 defines "terminology" but it uses a whole range of terms that are undefined, such as PolicyScript, accessor functions, policyCondition, and so on.
7) section 5.2, the paragraph starting "The Element Type Registration" has some text about determining the index portion. I think it would benefit from an example.
8) The last paragraph of section 5.2 about the impact of bad OID formation could use examples.
9) Throughout the document, the phrase "Note that ..." is used. This adds no value to most of the sentences where it is used. Upon first encountering this, I expected it to point out something especially important to note, but it seldom does, and is so overused it is very ineffective.
10) section 5.3 seems self-conflicting:
"no state is remembered from the previous invocation" and ""it had not matched that condition the last time it was checked"
11) section 5.3.1 "policy conditions are scheduled" - confusing and ambiguous. There has been no discussion of scheduling yet. There are two types of scheduling involved in this document - the mib-controlled schedule of evaluation/action, and the underlying implementation scheduling of the execution of evaluation/action processes. 
12) section 5.4.1 same issue as 5.3.1
13) section 5.5 contains Definitions - i.e. another section for terminology. It would seem reasonable to merge 5.1 and 5.5.
14) section 5.5 is entitled Definitions. So is section 11 (which contains the MIB Module). 
15) section 5.5. contains terminology definitions that use lots of forward references to other undefined terms. For example, "Ready Policy" has to with scheduling, but scheduling hasn't been discussed yet.
16) "If their are multiple" s/b there
17) section 6. "however it was desirable to have access to C++'s operator overloading (solely to aid in documenting the language - operator overloading is not a feature of PolicyScript" - huh? how do we have access to it if it isn't part of the language? how are you using it to document the language? 
18) "Some examples of the features that have been removed" - is the language likely to change based on what happens in SMING? For example, if SMING adds floating point to mib defintions, how will PolicyScript handle this in a backwards compatible way?
19) "For example, it is expected" doesn't need the "For example"
20) "This is done because while" - you only need "While"
21) "One subset is not expressible" - this could use a better definition of scope.
22) Does the postfix_expr rule allow ++A++, or A++++++, or A++--++?
23) How does the expression rule work with a comma? (A=0,B=1) evaluates as what? Am I allowed to specify A[A=2, B=3]?
24) iteration statement - what is the evaluation order in a for clause?
25) "PolicyScript: statement*" is a script with no statements a valid script?
26) Is the index specification ($*) included in the EBNF?
27) "inside of" can be just "in"
28) 6.2.1 "The policy script runtime" discusses conversion operators that are not part of the language. See item #17 above.
29) type conversion rules - the format of the rules isn't defined; users can figure it out, but it could have been made more obvious.
30) type conversion rules - are these in precedence order? It doesn't say so. What is the precedence order of operators?
31) If I understand correctly, the following will result in a RTE.
{ J="J"; K="K"; J=K } Is that correct? the intent?
32) If I read this correctly, A+B and A<B might use different conversion rules, if A=1 and B="2". 
A+B --> 1 + "2" --> "1" + "2" --> "12"
A<B --> 1 < "2" --> 1 < 2 --> true
Is this desirable? Is this consistent with C, C++, perl, etc?
33) while(A): needs a closing parenthesis.
34) "getvar("ifSpeed.$*") < 128000" - why are the outer quotes needed in the example? 
35) section 6.3 - I understand the intent is that programmers that know other langauges could pick this up quickly, but the guide references all sorts of things that have not been discussed - function calls, library routines, etc. Forward references to inSubtree(), ev(), WriteVar(), etc. I think the Quickstart Guide might be better placed after the language elements and library routines have been described (or in an appendix). 
36) on a related note, "$*" indexing has not yet been discussed, but is used in the Quickstart guide. Many programmers that know, say, C won't know this langauge element.
37) on a related note, "Because Policy Script is a least common denominator" isn't quite true. It contains elements, like $*, that are not in all the languages referenced. 
38) 6.3.1 - can accessor functions change the value of arguments that are not &references? 
39) 6.3.1 - are all variables in the same scope? what about scratchpad variables?
40) 6.3.1 - doesn't mention the addition of $*
41) 6.4 - "If a expression" 
42) the return value of a PolicyAction script is ignored. Why? why not use the return value to indicate whether the script ran to completion or not?
43) if the return value is ignored, why set it to zero as the result of an RTE or indication of a fail()?
44) 6.4 - discusses access functions, the defer attribute, and lower precedence scripts, before there has been any discussion of access functions, the library functions or script precedence.
45) section 7 - if SMING changes addressing to use non-OIDs, will snmpconf be able to be updated in a backwards compatible way?
46) "must be as appropriate" --> "must be appropriate"
47) "Any failure on this coercion" - should this be of rather than on?
48) "this coercion will cauae an RTE (in particular ..." The whole phrase in parentheses isn't needed.
49) "Arguments without the '&' character cannot be modified by the function." see item#38
50) section 9.1.1 - text starts with a '>'
51) There are a number of parameters that are based on enumerations defined in other documents, but the reference isn't specified here. Most notably, SNMPv3 documents define some, and SNMPv3 documents are already referenced for the boilerplate, so there isn't much reason not to specify the reference here.
52) "identified by the security credentials of the last entity to modify" - is this spelled out clearly somewhere? (I haven't finished the whole document, so it might be a forward reference. if so, can we specify the section?)
53) "the agent" - must it be an agent? does it apply to mid-level managers as well?
54) "to direct this operation at." ain't good English.
55) It might be worthwhile mentioning that if contextEngineID is not provided, and a managed environment supports dynamic addressing or network address translation, it may not be possible to reach the entity at the specified address. 
56) 9.1.2 enum_decimal - are enums required to start with a letter? How would ToInteger( 3Com(12) ) be evaluated? is the processing clearly defined to prevent mis-interpretation?
57) can unsigned64 types with the high  bit set be converted reliably using ToInteger()?
58) "unencoded" "encoded" - what does this mean? not ASN.1 encoded? 
59) subid: '0' | decimal_constant - why not use digit?
60) "never used in these encodings over the wire" - can we replace "these encodings" with "the scripts"?
61) getVar(...context...) - should this be contextName? also other places.
62) searchColumn - POSIX 1003.2 isn't mentioned in the References section
63) searchColumn() - "or returns an error without finding a match" - what are the error conditions? Since a zero means both endofView and error, how can a programmer determine that they have reached the end of the table without error?
64) searchColumn() - "This sends a getnext ... and continues to walk the tree until a value matching is found ..." and "In a loop, this looks simply like: while(searchColumn("ifType", oid, "6", 0)) ..."
The nested iteration makes this seem to be ambiguous about who handles iteration. I recommend the text read "In a loop to determine all the ethernet interfaces,".
Can this script be written as
while(searchColumn("ifType", oid, ethernet(6), 0)) ?
65) "There may be no MIB compiler (or MIB) available on the policy MIB agent" - The MIB is the virtual database containing managed objects (hasn't Jeff Case gotten that through your heads yet?! ;-). Obviousy this needs work; I don't have a strong suggestion. It might be easier to say that the entity has no capability for translating from descriptors to object identifiers.
66) "available on the policy MIB agent." might be better as "available on the entity containing the policy MIB." (containing might also be changed to enforcing.)
67) "too short" and "too much" - remove the "too"
68) createRow() - "The '*' must replace any integer index item that may be set to some random value." - this is a bit unclear. each integer index is not really set to some random value. A random value is selected, then each of the specified integer indexes is set to the same selected value.
69) counterRate() - somewhat ambiguous about how many, and which, timestamped values must be kept. The paragraph starting "counterRate retrieves" says it keeps "values" so it can calculate over a longer period than since the last invocation. The paragraph that starts "The implementation will need" says it must retain one value older than minInterval seconds. So, if I have a minInterval of 5 seconds, do I need to keep the last read (less than 5 seconds ago) and one more read?
What are the interoperability issues if I set two boxes to have minIntervals of 5 seconds, and vendor A keeps the most current and the most current + 5, but vendor B keeps the most current and the most current + 15 (discarding the current+5 and current+10 samples?).
70) is there a way to tell how many samples have been kept? 
71) is there a way to access the samples? their timestamps?
72) The description of the minInterval parameter is spread among multiple paragrpaghs. It really should have its own paragragh.
73) "Otherwise, this delta will be dividied by the number of seconds elapsed between the two retrievals ..." assumes there are two retrieveals. What if an implementation keeps more than two? how will the value be calculated?
74) "the integer-valued result will be returned." is this rounded up or down?
75) could discMethod be specified as an enum in this text, or could the text be formatted in bullets, to simplify reading the description? 
76) There are three examples for this call. The third is labeled "Policy which executes every 60 seconds", but the example shows a retrieval of the current delta. It is immaterial how frequently it executes.
77) 9.1.4 The paragraph starting "Note that in the preceding example," really isn't needed. This has already been discussed.
78) newPDU() - can a PDU that is received in a trap be read using the functions described here? Obviously, an implementation could read the information from the PDU, then create a newPDU() and write the info into the new PDU, but is there any way to "overlay" a newPDU() on an existing PDU to make the data accessible to the script? And would such a thing be allowed, given the stricture in 9.1.4 - "It is an RTE to read a varbind that has not been previously written."?
79) 9.1.4.4 is snmpSend() synchronous or asynchronous? This is important to know for off-box queries.
80) It is my impression that developers think of varbinds as being name-type-value rather than name-value-type. I cannot find any text that explicitly states this ordering for a varbind, except that ASN.1 works as type-length-value order. It might be easier to remember which parameter is which if reordered to match the ASN.1 ordering. In a language with defines or named constants, this wouldn't be a problem, since a call would be readVar(ifType, ethernet(6), Integer32), but these scripts must use the integer value of the datatype and (for an integer) the integer value. So the script will be 
readVar(1.3.6.1.2.1.2.2.1.3, 6, 4). People writing or reading scripts will need to keep these two parameters straight. Putting them in ASN.1 order might make it easier to remember the order.
81) 9.2 Are the HC datatypes supported? What does counterRate() return for a 64-bit counter delta?
82) "local" errors - when acting as a MLM, are these local errors or remote errors? Do we have remote errors?
83) Constants for SnmpMessageProcessingModel and SnmpSecurityModel - if additional security models are defined, can they be supported in snmpconf? A reference to the official definition of these values would be better than "hard-coding" the list here. Ditto for SNMP operation types, SNMP errors, and other constants. It's ok to include the constants here, but there should be a disclaimer that the official list is found in [wherever].
84) RFC1905 defines the error codes using standard SNMP naming practice for capitalization. That practice isn't followed here, at least not consistently. Undofailed should be undoFailed, and so on. 
85) Even if the WG decides not to be concerned with matching RFC1905, they should at least try to be internally consistent - compare Undofailed and InconsistentName.
86) SnmpMessageProcessingModel and SnmpSecurityModel is assigned enumerations in an inconsistent way in RFC2571. The constants here assume the values are consistent between ProcessingModels and SecurityModels, so these constants are incorrect.
87) 9.3.1 through 9.3.6 could be reordered to be easier on the reader. The first functon described depends on the function descriptions that follow it, but none of them depend on releMatch. I suggest reordering in this order: elementName(), elementAddress(), elementContext(), then ev() and ec(), then roleMatch().
88) throughout the descriptions, the context argument is described in three states: present, not-present, present but empty. I think there is a matter of degree here, and the descriptions should be ordered as: present, present but empty, and not-present.
89) ec() - "the number index subidentifiers exist" - obviously missing "of", but also not as clear as it could be. 'the number of suboids in the OID index for "this element".' I think there is a danger that implementers will count the subids in the complete OID, not just the index portion of the OID. An example showing an elementName OID and the OID index portion of the elementName and what ec() counts would help.
90) elementContext() is provided, and elementAddress() is provided, but elementContextEngineID() is missing, which is especially notable given that roleMatch() uses contextEngineID rather than the address.
91) roleMatch() ues contextEngineID and doesn't permit using the address, but nonLocalArgs favors the address over contextEngineID (the contextEngineID in nonLocalArgs can be not-present, and commands will be sent to the address specified, which is in keeping with SNMPv3 engineID discovery).
92) 9.3.7 The whole scratchpad discussion would benefit from being put into a separate section that includes a discussion of the various scopes, rather than doing it as part of the function definition.
93) setScratchpad() - I recommend that the sentence starting "The value of 'scope' " begin a new paragraph.
94) I think the texet in section 5.3 was written before the scratchpad was added, and they seem to be in conflict about remembering state.
95) PolicyElement scope discusses the fate-sharing with policies, but not with elelements. What happens if the current element goes away?
96) can storageType be changed for an existing variable?
97) "Contents of the scratchpad are erased on reboot." - doesn't this contradict the purpose of storageType?
98) 9.3.8 The examples ar einconsistent with the definitions. 
getScratchpad(Global, "foo", val) == 55 will never be true because getScratchpad only returns a 0 or 1 value.
99) I am unclear on how to specify a modifiable parameter in a function call - is it the C-style (&val), where the address of the variable is provided, or the C++-style (val), where the reference is defined in the prototype?
100) Is it an RTE if I specify getScratchpad(Global,"foo", "55")? The string "55" is associated with a memory location, just as val is, but the memory location for "55" may be read-only memory. Writing to it might cause a core dump. Alternately, a manifest constant string might be modified, making the rest of hte program malfunction. 
101) 9.3.9 defines constants for use with the scratchpad functions. Shouldn't these constants be defined with the other constants?
102) 9.3.10 "used to by"
103) signalError() - the clarity of the sentence starting "This bit is initially cleared" could be improved. It may be enough just to stop the sentence at "This bit is cleared at the beginning of each execution." Are all the bits in pmTrackingPEInfo cleared at the start of each execution? How do I know whether conditionUserSignal or actionUserSignal will be set? Assuming it gets set depending on whether a condition or action is being evaluated, does the conditionUserSignal get cleared when the action is evaluated, or when the next condition is evaluated? 
104) defer() - In the description, the order of discussion is default behavior, defer(1), then defer(0). is the default behavior and defer(0) equivalent? if so, they should be discussed together.
105) defer() - the discussion that preceds the prototype talks about gold is appropriate but isn't possible. It doesn't say anything about hitting a RTE. But the description in the prototype is dependent on hitting an RTE. Is it always true, and a restriction that any future library developers must be aware of, that an RTE MUST be triggered if it isn't possible to do something? 
106) Can a script writer choose to voluntarily, as part of their logic, fail() and defer()? Is that an RTE, or just returning a false condition? 
107) there has been no prior discussion of precedence groups.
108) 9.3.13 text starts with a '>'
109) getParameters() - given no knowledge of the MIB module, or any discussion which follows this text, this description is close to incomprehensible. "These parameters may be installed with the script in this object" - huh? what object? installed? with the script? what are you talking about? Obviously, the description from the object was cut and pasted here. 

my $.02

I have done through 9.3; more later,
dbh