[midcom] Re: midcom revised mib review

Hi Juergen,

Many thanks for the review!

Martin and I went through your comments and fixed the draft.
Please find our replies inline below.
Please have a look at issues h), m), n), and s).

Thanks,

    Juergen Q.

--On 31.05.2006 15:08 Uhr +0200 Juergen Schoenwaelder wrote:

> Hi,
>
> first of all, I have to apologize for the delay (I will spare you the
> reasons you might not be interested in anyway).
>
> I have read <draft-ietf-midcom-mib-06.txt> once again carefully. I
> think it is generally of high quality and I do not have major problems
> with it. Still, I have a few comments I like to share with you and
> perhaps some of the things can still be improved without much further
> delay.
>
> a) p6: "sending notification to MIDCOM client" - either "a MIDCOM
>        client" or "MIDCOM clients"

fixed.  We chose "MIDCOM clients".

> b) p13: "for request message and reply message" -> "for the request
>         and the reply message"

fixed.
> c) p17: "For example, it can" -> "For example, they can"

fixed.

> d) p22: "The values provide" -> "The values provided"

fixed.

> e) p26: "the idle or" -> "the idle time"

fixed.

> f) p27: "MIDCOm" -> "MIDCOM"

fixed.

> g) p29: "For some application" -> "For some applications"

fixed.

> h) p29: I am wondering why the document does not suggest to use
>         snmpSetSerialNo to solve the idempotency problem.

Actually, we did not find any MIB module using this feature.
Therefore, we do not expect big support form implementations.

Also, its DESCRIPTION clause

           "An advisory lock used to allow several cooperating
           command generator applications to coordinate their
           use of the SNMP set operation. ...

says that it is to be used for coordinating between multiple
command generators.  Our idempotency problem is rather related
to re-sent requests from a single command generator.

> i) p30: "below recommended" -> "below are recommended"

fixed.

> j) p31ff: I am still somewhat concerned about the fact that the text
>           encourages implementors to potentially do the wrong thing.
>           You can't rely on notifications. While step 7. in section
>           7.3 says the right thing, I would prefer if the text would
>           actually be moved where it belonts, namely in step 4. So
>           here is what I propose how step 4 should be written:
>
>    4. The MIDCOM client awaits a midcomSolicitedRuleEvent notification
>       concerning the new policy rule in the midcomRuleTable.  Waiting
>       for the notification is timed out after a pre-selected maximum
>       waiting time. In case of a timeout while waiting for the
>       notification or if the client does not use notifications, the
>       MIDCOM client retrieves the status of the midcomRuleEntry by one
>       or more SNMP get operation.
>
>           By moving the text into this step, you can get rid of step 7
> 	  and make it clearer that you have to write code to poll for
> 	  the completion of the operation anyways.
>
> 	  Note that this change also affects subsequent sections,
> 	  namely 7.4, 7.5, 7.6, and 7.10.

done for all of them.

> k) p33: Step 5 in 7.5 refers to step 5 in 7.3 but I think it should be
>         step 4 in 7.3.

fixed.

> l) p42: The description of midcomRuleOwner is a bit confusing. It says:
>
>             This object SHOULD uniquely identify an authenticated
>             MIDCOM client.  It is of type SnmpAdminString, a textual
>             convention that allows for use of the SNMPv3 View-Based
>             Access Control Model (RFC 3415, VACM) and allows an
>             management application to identify its entries."
>
>         This sounds like the SnmpAdminString TC has something special
>         concerning VACM which is not true. Perhaps less is more:
>
>             This object SHOULD uniquely identify an authenticated
>             MIDCOM client. This object is part of the table index to
>             allow for the use of the SNMPv3 View-Based Access Control
>             Model (RFC 3415, VACM).

done.

> m) p42: Should 0 not be excluded from midcomRuleIndex? You have done
>         this for the midcomGroupIndex.

In an earlier version we had a special semantics for references to groups
with index 0.  therefore, we excluded 0 from the set of values for
midcomGroupIndex.  When we removed this feature, we forgot to include value
0 again.

Consequently, the suggestion is allowing 0 also for midcomGroupIndex.

Would there be a problem with using index 0 for midcomRuleIndex and
midcomGroupIndex?

> n) p44: The description of midcomRuleOperStatus says that setting(2)
>         indicates that no request was made. I am not sure how this can
>         actually happen since midcomRuleAdminStatus is either
>         reserve(1) or enable(2) and hence during row creation it has
>         to take on one of these values. Perhaps a cleaner way to deal
>         with this would be to add another state off(3) or whatever you
>         want to call it which can be used when a row is created
>         without already setting reserve(1) or enable(2). The other
>         option would be to be very clear that a midcom client has to
>         set either reserve(1) or enable(2) while creating a row in
>         which case the state setting(2) does not make much sense to
>         me.

We added value notSet(3) to this object and the following paragraph to
the DESCRIPTION clause.

         When created an midcomRuleEntry is created without
         explicitly setting this object, its value will be notSet(3).
         However, a set request can only set this object to either
         reserve(1) or enable(2).  Attempts to set this object to
         notSet(3) will always fail with an inconsistentValue error.

> o) p45: "can be retrieved" -> "are meaningful" (twice on that page).
> 	My understanding is that you can always retrieve all columns
> 	but depending of the midcomRuleAdminStatus only some of them
> 	are meaningful.

fixed.

> p) p46: "rule is expired" -> "rule has expired"

fixed.

> q) p48: midcomRuleError does not support localization and this seems
>         to be a string that is likely to be shown to human users. I am
>         just mentioning this...

Thank you.
We do not see a straight way to supporting localization here.

> r) p54: Is there a reason why you do not use the
>         InetAddressPrefixLength TC for
>         midcomRuleInternalIpPrefixLength and
>         midcomRuleExternalIpPrefixLength? In case you use
>         InetAddressPrefixLength, make sure you allocate the max value
>         for the "no wildcarding" semantics and not just 128.

replaced
    SYNTAX Unsigned32 (0..128)
with
    SYNTAX InetAddressPrefixLength
for both objects.

> s) p63: How does midcomConfigPersistentRules interact with the
>         StorageType objects? In the light of StorageType, the term
>         "persistent" is probably wrong and should be "nonVolatile".

In all discussions of related issues on the mailing list, the term
"persistent" was used instead of "nonVolatile".  Therefore, we used
it for naming this object.  If you insist we can change it to
midcomConfigNonVolatileRules, but we prefer the current name.

Anyway, we appended the folowing paragraph to the DESCRIPTION clause.

         A value of true(1) indicates that there may be
         entries in the midcomRuleTable with object
         midcomRuleStorageType set to value nonVolatile(3).

> t) p63: First sentence in the DESCRIPTION is missing some words.

fixed.  Just one word missing.

> u) p67: "inteface" -> "interface"

fixed.

> v) p73: "request. (" -> "request ("

fixed.

> w) p74: "request. (" -> "request ("

fixed.

> x) p81: "to managed object" -> "to the managed object"

fixed.

> /js
>
> --
> Juergen Schoenwaelder		    International University Bremen
> <http://www.eecs.iu-bremen.de/>	    P.O. Box 750 561, 28725 Bremen, Germany

_______________________________________________
midcom mailing list
midcom@ietf.org
https://www1.ietf.org/mailman/listinfo/midcom