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

snmpconf-pm-04 notes continued




Having finished reading the draft, here is another slew of notes I
jotted down while reading it.  Again, I'll mark the more serious
issues (in my mind) with a "***" (ie, those more theoretical rather
than editorial and require more major structural changes though my
consistency for marking them probably isn't perfect).

Since I'm proposing a huge number of changes at this point (62 in
all!), I'd be happy to edit any deemed acceptable into the document if
you'd rather I do the work than one of the editors (I'll need the
source if thats the case).

***
22) The most blunt of my statements today:  The pmNotificationRegTable
    is redundant with the existence of the NOTIFICATION-MIB and, IMHO,
    should be dropped from the mib entirely (but, obviously, the
    notifications definitions themselves should remain).  Duplication
    of standardized work is a bad thing.

23) The mib is sprinkled with text in the form of comments that
    describe the table(s) below them.  This is a pet peeve of mine, as
    I frequently use mib parsing tools to browse through mib
    descriptions where I'd expect that information to be properly
    kept.  In particular, I'd like to suggest that the text preceding
    the following mib items should be moved into the items themselves:

       pmRoleESTable [this one has to be done carefully, as it
                     describes two tables not just one]
       pmElementTypeRegTable
       pmCapabilitiesTable
       pmDebuggingTable
       pmNotificatioonRegTable [unless this table is dropped; see above]
       
    I'd actually suggest a note be put into the -bcp document as well
    describing that comments should never be used to describe
    functionality of objects, but rather to mark things to remember, etc.

24) the capMatch function definition is not clear as to what the OID
    should be.  After reading everything, I'm guessing that it should
    be an oid corresponding to a pmCapabilitiesType definition, but I
    could easily accidentally read it as possibly a
    pmCapabilitiesSubType OID or even a row pointer into the
    pmCapabilitiesTable itself.

    (while I'm at it, the capabilities table seems almost redundant
    with respect to the sysORTable.  There is a lot of overlap and it
    took me a while to figure out why both are needed: the table
    defined here references mib accessible changes, while the
    sysORTable contains only a pointer to a mib object.  I'd be
    tempted to write a sysORTable agent-capabilities mib table instead)

25) section 5.3 implies that an action is always to be run after a
    filter has evaluated to true, but 5.2 dictates that it's only to
    be executed within the time frame of policyFilterActionMaxLatency.
    It's possible that the action will not be run given a filter's
    output if the corresponding value of policyFilterActionMaxLatency
    is greater than twice the of pmPolicyFilterMaxLatency.  Acutally,
    the whole issue of those two objects is rather odd.  The way I see
    it, these two objects allow for two different management functions
    to be run separetly:

      1) runs a filter at least as frequently as
         pmPolicyFilterMaxLatency and remembers the results.
      2) runs a set of actions based on the results from #1 above with
         at least the frequency of policyFilterActionMaxLatency.

    The odd thing is that these items allow you to configure the row
    in very odd ways, like the example described above where 

      policyFilterActionMaxLatency > pmPolicyFilterMaxLatency*2

    which might produce both cases of an action not running for every
    filter check, and a case where an action may not get run at all
    (if a filter evaluates to true then false during the second
    reading).  (I guess 5.3 also states it must be run immediately
    upon the first discovery of a filter that returns true, but future
    invocations may not happen).

    Another odd state would be where

      policyFilterActionMaxLatency < pmPolicyFilterMaxLatency*2

    which would mean that the action would run even though a recent
    check hadn't been performed to make sure it should be run (which,
    I guess, is not *that* odd).

    So if an implementation didn't want to separate these two phases
    of collection and responding, they should do filtering/action
    running based on min(policyFilterActionMaxLatency,
    pmPolicyFilterMaxLatency)?

***
26) setRowStatus() comes up with a random index based on maxTries, but
    is somewhat inefficient if some knowledge of the table exists
    already.  I'd really like to specify a starting value as well, in
    case the table contents were exceptionally large (and I'd default
    to 0 if I knew nothing about the contents).  If I need to create 5
    rows, I'd like to start my 2nd-5th calls searching with the last
    assigned value from the previous runs.

***
27) Its still unclear to me when in the API (if ever) you're allowed
    to reference oids by textual name.  I'm gathering that never is
    the case for all APIs?  If so, I'd replace the examples (like
    11.1.2) that reference textual oids with numeric ones (even if you
    state earlier than some management apps may convert them for you
    before uploading the code).

28) 11.1.2.2
        type is the type of the value parameter and will be set to
        one of the values for DataType Constants.
     ->
        type will be the type of the value parameter and will be set to
        one of the values for DataType Constants.

***
29) snmpsend() should really really really pass back the errindex (into
    a new parameter).  Efficiently parsing through a huge varbind list
    to find the source of the error would be otherwise impossible.

***
30) I'd suggest a new function called "logDebugString()" that would
    log a string to the pmDebuggingTable if the row was configured
    with debugging turned on (otherwise it'll result in essentially a
    noop()).

31) It's kind of odd that you declare the constants as "const" even
    though the language doesn't allow the "const" token.  I'd be
    happier if the document just declared that all constants can be
    treated as an integer and simple list the name and value pairs
    without trying to translate them into C-like statements at all.

***
32) I think all the constants should be matched with the similar BER
    tag encodings where possible (just for ease-of-implementation
    issues).  For instance, a string shouldn't be const "2" but rather
    "4".  Similarly, the error constants like NosuchObject should
    probably be "128" instead of "21", etc.  Non-snmp related
    constants should then be moved further away from the most likely
    places for the SNMP protocol spec to declare future additions (bad
    parameter might move to 32767 or something).

33) setScratchpad(varName, scope, value) should have its arguments
    reordered to (scope, varName, value).  Just makes more sense,
    IMHO, to have name and value next to each other.

34) Many functions like getScratchpad, for example, don't state what
    will be returned in variables like getScratchpad's value argument
    when an error has occurred.  I'd suggest explicitly stating that
    the value is unchanged from before the call, or that it is
    undefined (I'd prefer it remain the same for consistency of
    programming).

**
35) Throughout the document strings and integers are frequently mixed
    and shouldn't be unless this is legal (which from my reading, I
    don't think it is).  The "Scratchpad Usage Examples" section, for
    instance, has the return values of the getScratchpad call being
    '== 55' for example, when it should really be '== "55"'.

    I'd be somewhat happier as a scripting programmer if the values
    were indeed inter-usable (automatically converted as needed), as
    perl's variables are (since you reference it earlier in the
    document).

36) oidncmp() has an argument "int n" that isn't defined in the text.
    I assume its the length against which to compare the oids in
    question.  So if I want to compare 2 oids to their completion, do
    I:

    a) oidncmp(oid1, oid2, 0)
    b) oidncmp(oid1, oid2, 128)
    c) int o1, o2, otmp;
       o1 = oidlength(oid1);
       o2 = oidlength(oid2);
       if (o1 > o2)
         otmp = o1;
       else
         otmp = o2;
       oidncmp(oid1, oid2, otmp);

37) subidwrite() and subid() says their  "int n" argument must
    start at "0".  I'd suggest replacing the argument instead with
    "unsigned int n", which solves the problem the correct way (and
    forces a complier check).  In fact, I think all the function
    definitions could probably use a once over for this type of
    checking.  The "int subid" of subidwrite(), for example, is
    another case where it should probably be an unsigned int (since an
    oid can't have negative values in it).

38) A note should be put in subidwrite() saying that appending is
    possible through the string concatinator operator (+).

39) the definition of oidsplice() is somewhat ambiguous.
    Specifically, "oidsplice replaces n subidentifiers in oid1 with
    all of the subidentifiers from oid2 starting at the m'th
    subidentifier of oid1".  So, does this mean that:

      oidsplice(".1.2.3.4", 2, ".5.6.7.8.9", 1)

    will result in ".1.5.6.7.8.9.3.4"?  Thats how I read it, but I
    don't know if thats correct.

40) library accessor functions uses atoi, but I'd rather have strtoul
    around instead if possible.

41) library accessor functions defines random, but doesn't state a
    minimum acceptability of random-ness provided by the function.
    Just a thought.

42) Why are memcmp() and memmove() in the library accessor functions?
    We have no pointers?

43) substr() would be a good type of function to add to the list of
    functions.

44) You should probably import UTF8String from SYSAPPL-MIB::Utf8String
    instead of defining your own.  Though you don't put a size
    restriction on yours, but yet your display hint has one???

***
45) You're tables are frequently indexed by arbitrary integer numbers,
    which is another pet-peeve of mine.  This number has no meaning
    what so ever and iterating through a table is always required if I
    need information.  I'd much rather that things be indexed by a
    string where possible.  For example, the pmPolicyIndex should be
    "pmPolicyName" instead and be an snmpAdminString (IMHO, of
    course).  Most of the other tables could use this type of indexing
    change as well.  I'd be happy to come up with a list if the idea
    is acceptable.

***
46) If pmPolicyPrecedence values must be unique, then I *highly*
    recommend making that the index (I know, I'm contradicting what
    I'm suggesting in #45 above but now the number would have
    significance).

47) A flow diagram or state table describing pmPolicyPrecedence is
    greatly needed.

48) Any reason why pmPolicyDescription can only be 255?  I'd be
    tempted to allow larger sizes (and implementations can reduce the
    length and document it in their agent-capabilities statements).

49) pmPolicyMatches: "The number of elements that are currently
    matched by the associated pmPolicyFilter".  When is this number
    determined?  At the time of the last filter run or does the filter
    run right now to come up with the number?

50) I don't understand the difference between
    pmPolicyAbnormalTerminations and pmPolicyExecutionErrors?

51) Why are pmPolicyAbnormalTerminations and pmPolicyMatches writable
    (and Gauges for that matter)???

***
52) pmPolicyStatus should indicate if any of the previous row columns
    are mandatory to set before coming active?  Certainly some objects
    need setting, like the pmPolicyPrecedence object before it can
    become operational.  Default values might be needed for some of
    the other objects as well (like the latency objects).

53) pmPolicyCodeText has a type "The code" in the middle of the
    description that probably is either supposed to be removed or
    added to the beginning of the next sentence.

***
54) pmElementTypeRegTable should be indexed by the
    pmElementTypeRegOIDPrefix OID in question to help managers
    determine if an entry already exists for that object.

    Additionally, I'd suggest that the index should point to the table
    object and a second "column number" object be created to indicate
    what column should be used when searching for sub-objects.  This
    would allow guaranteed uniqueness of the pmElementTypeRegOIDPrefix

55) pmElementTypeRegMaxLatency should give a pointer as to what to do
    after a new element is discovered.

***
56) pmCapabilitiesTable should be indexed by the type and/or subtype
    for faster traversal when you're looking for a particular
    capability.  (The type object really needs a better description,
    since I have no idea what its value should indicate other than it
    has been "assigned by IANA".)

57) pmTrackingElementToPolicyElement:
   "The element this policy is configuring" -> um...
     "The element that is configured by the policy defined in this row."

    I don't really like that either, though, but it does need to
    change, IMHO.

58) Why define "off" in pmTrackingElementToPolicyStatus (and
    pmTrackingPolicyToElementStatus) at all?  Why not just say that
    the row doesn't exist unless the policy is on.  The enum value
    will *never* get used.

59) Can we force creation of rows into the
    pmTrackingElementToPolicyTable by setting the
    pmTrackingElemenToPolicyStatus object to "forceOff"?  IE, so its
    definition is remembered when it comes active via the calendar
    later on?

***
60) most of these tables could use a "StorageType" object.

    (note, the suggested use of the StorageType object should probably
    be mentioned in the -bcp document as well).

61) A minimum/maximum number of support entries in the
    pmDebuggingTable should be defined somewhere (the maximum being a
    mib object returned by an object in the agent).  Also, roll over
    of the log entries is done by, what, time?  A time index would
    probably be a good thing for the table as well.

62) notification registration table coments, step 4:
    downloaded -> uploaded & download -> upload


-- 
Wes Hardaker
NAI Labs
Network Associates