RE: review of snmpconf--pm-10

Hi,

The items are from my review of draft-ietf-snmpconf-pm-08.
The section numbers in my mail and in the -10- document may vary slightly,
but not much.
I have separated items according to my percieved severity, to help you deal
with them quickly.
I moved some into "completed" that really weren't addressed, but I could
accept as is.
You might need to look up preceding items (in different lists) for the
context they provide.

Issues of clarity or conflict that could impact useability or
interoperability:

8) The last paragraph of section 5.2 about the impact of bad OID formation
could use examples.
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
39) 6.3.1 - are all variables in the same scope? what about scratchpad
variables?
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()?
49) "Arguments without the '&' character cannot be modified by the
function." see item#38
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?)
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()?
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?
79) 9.1.4.4 is snmpSend() synchronous or asynchronous? This is important to
know for off-box queries.
81) 9.2 Are the HC datatypes supported? What does counterRate() return for a
64-bit counter delta?
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].
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.
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).
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 elements. What happens if the current element goes away?
96) can storageType be changed for an existing variable?
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. 
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? 
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?  

Questions the document doesn't answer that could impact interoperability:

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?
38) 6.3.1 - can accessor functions change the value of arguments that are
not &references? 
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?
45) section 7 - if SMING changes addressing to use non-OIDs, will snmpconf
be able to be updated in a backwards compatible way?
53) "the agent" - must it be an agent? does it apply to mid-level managers
as well?
70) is there a way to tell how many samples have been kept? 
71) is there a way to access the samples? their timestamps?
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."?
82) "local" errors - when acting as a MLM, are these local errors or remote
errors? Do we have remote errors?

Editorial Suggestions - not important to interoperability, just document
quality.
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"?
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.
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?
19) "For example, it is expected" doesn't need the "For example"
20) "This is done because while" - you only need "While"
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. 
40) 6.3.1 - doesn't mention the addition of $*
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.
46) "must be as appropriate" --> "must be appropriate"
48) "this coercion will cauae an RTE (in particular ..." The whole phrase in
parentheses isn't needed.
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.
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"?
62) searchColumn - POSIX 1003.2 isn't mentioned in the References section
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"
72) The description of the minInterval parameter is spread among multiple
paragrpaghs. It really should have its own paragragh.
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.
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.
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.
101) 9.3.9 defines constants for use with the scratchpad functions.
Shouldn't these constants be defined with the other constants? 

Completed in -10-
1) the format of the first page header doesn't follow rfc2223, and seems
different than most.
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.
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
21) "One subset is not expressible" - this could use a better definition of
scope.
33) while(A): needs a closing parenthesis.
34) "getvar("ifSpeed.$*") < 128000" - why are the outer quotes needed in the
example? 
41) 6.4 - "If a expression" 
47) "Any failure on this coercion" - should this be of rather than on?
50) section 9.1.1 - text starts with a '>'
54) "to direct this operation at." ain't good English.
61) getVar(...context...) - should this be contextName? also other places.
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)) ?
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?).
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? 
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.
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().
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.
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.
102) 9.3.10 "used to by"
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.
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.