[ntpwg] My review of: draft-ietf-ntp-ntpv4-mib-05.txt
Odonoghue, Karen F CIV NSWCDD, W23
karen.odonoghue at navy.mil
Thu Feb 19 17:06:14 UTC 2009
Heiko,
I'm driving and need to answer via blackberry. The short answer is we need to make the changesd. Bert is part of the official review process. I finally go to Chris. Email him at chelliot at pobox.com. I will give you a more coherent answer when I arrive at my destination.
Karen
----- Original Message -----
From: ntpwg-bounces+karen.odonoghue=navy.mil at lists.ntp.org <ntpwg-bounces+karen.odonoghue=navy.mil at lists.ntp.org>
To: Bert Wijnen (IETF) <bertietf at bwijnen.net>
Cc: ntpwg at lists.ntp.isc.org <ntpwg at lists.ntp.isc.org>
Sent: Thu Feb 19 02:19:57 2009
Subject: Re: [ntpwg] My review of: draft-ietf-ntp-ntpv4-mib-05.txt
Bert,
thank you very much for your detailed and useful review of our MIB
module. I will try to implement as many points as possible but I am
unsure what that does to the standardization process (it is my first RFC
....). It took us all a long time to get into last call and I am a
little bit scared that the modifications will restart the whole
procedure again. I asked my wg chair for advise what to do.
See my additional comments below (inline):
Bert Wijnen (IETF) schrieb:
> Note: I am not subscribed to the ntpwg mailing list and do not plan to
> subscribe either. So if you want me to see any responses, please
> copy me explicitly
>
> I did a review for:
> http://www.ietf.org/internet-drafts/draft-ietf-ntp-ntpv4-mib-05.txt
>
> Here are my findings:
>
> SMICng results:
>
> C:\bw\smicng\work>smicng ntpv4.inc
> E: f(ntpv4.mi2), (272,14) Default value for "ntpEntStatusCurrentModeVal"
> must be a name and not a number
>
> *** 1 error and 0 warnings in parsing
>
> This means changing
> DEFVAL { 99 }
> into
> DEFVAL { unknown }
> will fix it.
Strange, I used smilint at level 6 and did not get any errors. This is
an easy fix, though.
> But I also find:
>
> - writable objects for which I see no text in the DESCRIPTION clauses
> about expected persistency behaviour
That means you want to know whether an object stores the changes on that
object permanently or not? I am not sure if this is not
implementation-specific.
> - Counter32 objects with no test in the DESCRIPTION clauses about
> possible discontinuities and which timer object would indicate such.
> Specifically, the fact that (I think) the ntpd keeps track of the
> Counters,
> it seems that there may be discontinuities at other times than when
> the snmpd restarts (e.g. at other times than reset of sysUpTime)
Not sure what you mean, can you please provide me with an example?
> - I see a whole list of REVISION clauses. Normally we only have a
> single revision clause at RFC publication time. Not a list of revisions
> that took place during I-D revisions.
OK.
> - I am missing the Copyright statement in the DESCRIPTION clause
> of the MODULE_IDENTITY
I guess this has been overseen but can be added quite easily. Where can
I find the required copyright statement in its correct form?
> - I am missing that this revision of the document is published as RFC
> xxxx
> (xxxx to be filled in by RFC-editor)
OK.
> - I see a lot of objects with a SYNTAX of DisplayString.
> I would like to point out "note 2" on web page
> http://www.ops.ietf.org/mib-common-tcs.html
> Note 2. DisplayString does not support internationalized text.
> It MUST NOT be used for objects that are required to
> hold internationalized text (which is always the case
> if the object is intended for use by humans [RFC2277]).
> Designers SHOULD consider using SnmpAdminString,
> Utf8String, or LongUtf8String for such objects.
The DisplayString objects are mainly used for values which require to be
floating point and we agreed in the working group discussions that it
would make sense to use a string instead that can be converted into an
fp value by a machine.
> - Instead of
> ntpEntNotifPrefix OBJECT IDENTIFIER ::= { ntpSnmpMIBObjects 6 }
> ...
> ntpEntNotifications OBJECT IDENTIFIER ::= { ntpEntNotifPrefix 0 }
>
> I propose/suggest to do
> ntpEntNotifications OBJECT IDENTIFIER ::= { ntpSnmpMIB 0 }
>
> This makes it more inline with what we often do in MIB module.
> Also makes the OIDs of notifications shorter
OK, agreed.
> - I strongly recommend to use the MIB structure as per RFC4181,
> which suggests that you would replace
> ntpEntConformance OBJECT IDENTIFIER ::= { ntpSnmpMIB 6 }
> with
> ntpEntConformance OBJECT IDENTIFIER ::= { ntpSnmpMIB 2 }
>
OK.
> - I see OBJECT-GROUP definitions like
> ntpEntObjectsGroup1 OBJECT-GROUP
> OBJECTS {
> ntpEntSoftwareName,
> ntpEntSoftwareVersion,
> ntpEntSoftwareVersionVal,
> ntpEntSoftwareVendor,
> ntpEntSystemType,
> ntpEntStatusEntityUptime,
> ntpEntStatusDateTime,
> ntpAssocName,
> ntpAssocRefId,
>
> ntpAssocAddressType,
> ntpAssocAddress
> }
> STATUS current
> DESCRIPTION
> "A collection of objects for the NTP MIB that all NTP
> or SNTP entities should implement."
> ::= { ntpEntGroups 1 }
>
> Text about "should implement" or "optional" or such is NOT supposed
> to be
> in an OBJECT-GROUP DESCRIPTION clause. The OBJECT-GROUP is
> only meant to group objects logically together.
> If such a group is mandatory or optional is something that is expressed
> via MANDATORY-GROUPS or via the GROUP clauses in the
> MODULE-COMPLIANCE.
OK, I will remove the text from the description and change the group
clauses accordingly.
> As another example, you have
>
> ntpEntObjectsGroup2 OBJECT-GROUP
> ...
> DESCRIPTION
> "A collection of objects for the NTP MIB that are optional
> for NTP or SNTP entities to implement."
> ::= { ntpEntGroups 2 }
>
> So here you say that the group is optional while you have listed this
> group
> as a MANDATORY-GROUP here:
>
> ntpEntNTPCompliance MODULE-COMPLIANCE
> STATUS current
> DESCRIPTION
> "The compliance statement for SNMP entities which use NTP and
> implement the NTP MIB"
> MODULE -- this module
> MANDATORY-GROUPS {
> ntpEntObjectsGroup1,
> ntpEntObjectsGroup2,
> ntpEntNotifPrefixGroup
> }
> ::= { ntpEntCompliances 1 }
>
> So that is a conflict in your MIB-Module.
OK.
> - I see a lot of MIB-level comment lines, aka:
>
> ntpEntSoftwareVersion OBJECT-TYPE
> SYNTAX DisplayString
> MAX-ACCESS read-only
> STATUS current
> DESCRIPTION
> "The software version of the installed NTP implementation."
> -- full version string, e.g. "ntpd-4.2.0b at 1.1433 ..."
> ::= { ntpEntInfo 2 }
>
> I think it is much better to include that informatrion in the
> DESCRIPTION
> clauses. Otherwise it will often be lost when the MIB module is
> extracted
> or used as input for generating help text etc.
Good point, noted.
> - I see references/citations aka:
>
> NtpDateTime ::= TEXTUAL-CONVENTION
> DISPLAY-HINT "4d:4d:4d.4d"
> STATUS current
> DESCRIPTION
> "NTP date/time on the device, in 128-bit
> NTP date format. Ref: draft-ietf-ntp-ntpv4-proto-06,
> section 6:
> It includes a 64-bit signed seconds field
> spanning 584 billion years and a 64-bit fraction
> field resolving .05 attosecond (i.e. 0.5e-18).
> For convenience in mapping between formats, the
> seconds field is divided into a 32-bit era field
> and a 32-bit timestamp field.
>
> If time is not syncronized this field shall be a
> zero-length string.
>
> This TC is not to be used for objects that are used
> to set the time of the node querying this object.
>
> NTP should be used for this--or at least SNTP."
> SYNTAX OCTET STRING (SIZE (0 | 16))
>
> I suggest that the pointer to Ref: draft-ietf-ntp-ntpv4-proto-06
> is changed to Ref: RFC yyyy (and ask RFC-editor to fill in
> that yyyy value. I see that Ref: draft-ietf-ntp-ntpv4-proto-11 is
> also in Last Call, so that would be fine.
> In genreal though, it is probably better to use the REFERENCE
> clause. So it would be something like
>
> REFERENCE "Network Time Protocol Version 4 Protocol
> And Algorithms Specification, sect 6,
> RFC yyyy"
>
OK, makes perfect sense.
> - It seems to me that for something like this:
> ntpEntStatusEntityUptime OBJECT-TYPE
> SYNTAX Unsigned32
> MAX-ACCESS read-only
> STATUS current
> DESCRIPTION
> "The uptime of the NTP entity in seconds."
> -- time since ntpd was (re-)started (not sysUptime!)
> DEFVAL { 0 }
> ::= { ntpEntStatus 9 }
>
> I would add a UNITS clause.
> I would also move the comment line inside the DESCRIPTION clause.
> I am not sure if a DEFVAL of 0 (zero) makes sense.
> Specifically... what would the value be if the snmp agent has started
> later that ntpd? What if the snmp agent restarts and the ntpd does not?
> In fact, I think that a DEFVAL clause makes no sense for this object.
Agreed, I will remove the DEFAL line. Can you provide me with an example
on how to use the UNITS clause?
> I in fact wonder if most of the DEFVAL values for the read-only objects
> make sense.
>
> - For
> ntpEntStatPktModeTable OBJECT-TYPE
> SYNTAX SEQUENCE OF NtpEntStatPktModeEntry
> MAX-ACCESS not-accessible
> STATUS current
> DESCRIPTION
> "The number of packets sent and received by packet mode."
> ::= { ntpEntStatus 18 }
>
> ntpEntStatPktModeEntry OBJECT-TYPE
> SYNTAX NtpEntStatPktModeEntry
> MAX-ACCESS not-accessible
>
> STATUS current
> DESCRIPTION
> "The number of packets sent and received by packet mode."
> INDEX { ntpEntStatPktMode }
> ::= { ntpEntStatPktModeTable 1 }
>
> I am surprised by the SAME DESCRIPTION clause for each of these!!??
I will change that.
> - When I see:
> ntpAssocAddressType OBJECT-TYPE
> SYNTAX InetAddressType
> MAX-ACCESS read-only
> STATUS current
> DESCRIPTION
> "The type of address of the association."
> -- contains the type of address for uni/multi/broadcast associations
> ::= { ntpAssociationEntry 4 }
>
> ntpAssocAddress OBJECT-TYPE
> SYNTAX InetAddress
> MAX-ACCESS read-only
> STATUS current
> DESCRIPTION
> "The IP address (IPv4 or IPv6) of the association."
> -- contains IP address of uni/multi/broadcast associations
> ::= { ntpAssociationEntry 5 }
>
> Then I like to point out that RFC4001 states that the use of InetAddress
> SYNTAX, MUST have a DESCRIPTION clause that explain./states
> which object of SYNTAX InetAddressType controls the format of
> the InetAddress object. It is most probably ntpAssocAddressType
> but it would be good to state so.
>
> Besides, if you are sure that this ALWAYS only applies to IPv4 and IPv6,
> then I would do it as follows:
>
> ntpAssocAddressType OBJECT-TYPE
> SYNTAX InetAddressType { ipv4, ipv6 }
>
> ntpAssocAddress OBJECT-TYPE
> SYNTAX InetAddress SIZE(4|16)
>
Thanks, I will do it this way because we currently only have NTP over
IPv4 and IPv6 - to my knowledge.
> You must check if you also want to use Zoned addressed, because then
> that must be added too.
Any thoughts on that by anybody?
> If, at the other hand, other values than ipv4/ipv6 are possible (maybe
> in the future?) then it might be better to put the "minimum support for
> ipv4/ipv6 requirement in the MODULE-COMPLIANCE via an OBJECT
> and SYNTAX clause.
Again, since this is my first MIB I would appreciate an example.
>
> - I see some 8 notifications. Is there a risk that too many get generated
> at any point in time, such that it would flood the network and/or
> overload a NMS ??
> It would be good to add some text about the expected max notification
> rates and/or add a throthle or an enable/disable switch.
OK, I will look into this.
> - It seems that RFC4001 is missing as a normative reference.
OK.
> - I would think that the new draft-ietf-ntp-ntpv4-proto-11 (version 4
> of NTP)
> should also be a normative reference
Shouldn't we use the RFC yyyy approach instead (yyyy being the
to-be-assigned RFC for the protocol RFC)?
> - The security section is not according to the guidelines provided at
> http://www.ops.ietf.org/mib-security.html
I unfortunately currently seem to be unable to access this website (and
www.ietf.org as well), but I think it would be helpful to know which
points are missing / wrong in your oppinion.
>
> - I have not checked the new NTP version 4 spec to see if the
> knobs/sensors
> in that protocol have properly been modeled in this MIB module.
> That is a separate task that may actually need much more time than the
> review I have done for now.
Since this has been discussed in the working group I would expect that
everybody is happy with what is in the MIB right now. But I really
appreciate your efforts and your very detailed review of this document.
I only wish we would have somebody sending me such a review earlier in
the process, before everything went into last call. Anyway, you did a
great job and I think this was a very useful review. I will go back to
my drawing desk and start to implement the advised changes.
Best Regards,
Heiko
> Bert Wijnen
_______________________________________________
ntpwg mailing list
ntpwg at lists.ntp.org
https://lists.ntp.org/mailman/listinfo/ntpwg
More information about the ntpwg
mailing list