Subject: [Gen-art] Gen-ART Telechat review of draft-ietf-tictoc-ptp-mib-08

From: Peter Yee <[email-masked]>
Date: Sun, 17 Apr 2016 13:18:35 -0700

I am the assigned Gen-ART reviewer for this draft. The General Area Review
Team (Gen-ART) reviews all IETF documents being processed by the IESG for
the IETF Chair. Please wait for direction from your document shepherd or
AD before posting a new version of the draft.

For more information, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-tictoc-ptp-mib-08
Reviewer: Peter Yee
Review Date: March 8, 2016
IETF LC End Date: March 8, 2016
IESG Telechat date: April 21, 2016

Summary: This draft is basically ready for publication as a Standards
Track RFC, but has nits and a couple of minor issues that should be
fixed/considered before publication. [Ready with issues]

This draft defines an SMIv2 MIB for use with the Precision Time Protocol
v2 (IEEE Std. 1588-2008).  I do not have access to this standard, so my
review primarily addresses nits within the module and not whether the
module itself is a fair representation of a MIB suitable for use with PTP
v2.

The following review was originally submitted for the LC (see
http://www.ietf.org/mail-archive/web/gen-art/current/msg13014.html).  As
no update has been issued for the draft, the content of that review is
copied below.

Major issues: None

Minor issues:

Given that the draft seems to quote a fair amount of text (at least 5
pages) from IEEE Std. 1588-2008 in the definitions section if not
elsewhere, it might be good to make sure that there are no copyright
issues.

Many of the object descriptions use identical text.  While that's helpful
from the direct lookup point of view, it doesn't help to distinguish these
items from others of their ilk.  I'm not sure if this matters, but perhaps
enhancing the descriptions in their contexts might make them more useful.

Nits:

General:

Replace all occurrences of "create logical group" with "create a logical
group".

Watch the case and spacing of object names in their descriptions.
Sometime there's a space between two words, sometimes not.  Some words are
capitalized in one place and not another.  Use of camel case conventions
comes and goes.  It's probably best to treat the object names as English
words in the descriptions, separating words with spaces and generally
writing them in lower case.  Sure, the inconsistency doesn't hurt a lot,
but it ends up looking a wee bit sloppy.  The RFC Editor may not be sure
how to handle the inconsistency as some word combinations might appear to
be technical MIB terms that should be left as is.

Drop the draft name from the center, top header.  Generally a shortened
form of the draft title is placed here.  Perhaps you could use "PTPv2 MIB"?

I'll leave the serial comma be.  It appears in some places and not others.
I like them, but there are enough missing ones that I'm just going to let
it slide. ;-)

When citing the official name of IEEE 1588-2008, use a lower case "a”
before "Precision".  That aligns with IEEE's title.

The authors' addresses in Section 9 have way too many punctuation marks in
them.  A few less commas would go a long way.  Maybe change some into
serial commas for sprinkling elsewhere in the document? ;-)

Specific:

Page 2, Section 1, 1st paragraph, 2nd sentence: delete "the” before
"ordinary".  Make "ordinary clock" and "transparent clock" plural.  Insert
a comma and "and" after "transparent clocks".

Page 3, Section 1.1, 1st paragraph, 2nd sentence: append a comma after
"e.g.".

Page 5, 1st bullet item: delete the comma after "STD 62".

Page 5, 2nd bullet item, 2nd sentence: insert "is" before "described".

Page 5, 3rd bullet item, 2nd sentence: insert "is" before "described".

Page 5, 3rd bullet item, last sentence: insert "is" before "described".

Page 7, MIB Description, 1st paragraph, 1st sentence: change "MIB is to
support" to "MIB supports".

Page 8, Acronyms, EUI: delete the period after the acronym expansion.

Page 12, PTP node definition: insert "A" before the sentence.

Page 16, ClockIdentity description: the term "MAC-48" has not been defined
or previously referenced in the draft.  It's also not quite an exact
synonym for EUI-48, so perhaps it should be defined.

Page 17, ClockMechanismType description: replace "End to End” with
"end-to-end".  Replace "peer to peer" with "peer-to-peer".

Page 19, ClockPortTransportTypeAddress description, 2nd sentence: change
"Transport" to "transport".

Page 19, ClockPortTransportTypeAddress description, 4th sentence: insert
"an" before "address".  Change "and" to "or".

Page 20, ClockQualityAccuracyType description: change "section” to
"sections".  Insert "of" before "RFC 2578", which should be placed in
square brackets.  Change "to start with" to "starting at".

Page 23, ClockStateType description: insert "a" before "PTP".

Page 24, Holdover state text: change "ptp" to "PTP.  Change "FREERUN” to
"Freerun".

Page 25, ClockTimeSourceType description: change "section" to "sections”.
Insert "of" before "RFC 2578", which should be placed in square brackets.
Change "to start with" to "starting at".

Page 26, ClockTimeSourceType reference: change "Section" to "Sections".

Page 32, ptpbaseClockCurrentDSOffsetFromMaster and
ptpbaseClockCurrentDSMeanPathDelay references: the indentation looks a bit
off here.

Page 32, ptpbaseClockCurrentDSMeanPathDelay description: change "measure”
to "measured".

Page 35, ptpbaseClockParentDSOffset description (near top of page): change
"clocks" to "clock's".

Page 35, ptpbaseClockParentDSClockPhChRate description: change "clocks” to
"clock's".

Page 41, ptpbaseClockRunningState description: insert "a" before "PTP”.
Append a comma after "engine".

Page 42, top: Rather than duplicate the state descriptions information
here, why not merely point back to ClockStateType?  Btw, PTP engine hasn't
been defined.

Page 42, Holdover state, 1st sentence: if you do not remove this paragraph
as suggested in the previous comment, change "ptp" to "PTP".

Page 42, ptpbaseClockRunningPacketsSent description: replace "packet
Unicast and multicast" with "unicast and multicast packets".

Page 42, ptpbaseClockRunningPacketsReceived description: replace “packet
Unicast and multicast" with "unicast and multicast packets".

Page 44, ptpbaseClockTimePropertiesDSCurrentUTCOffsetValid description:
insert "the" before "UTC".

Page 45, ptpbaseClockTimePropertiesDSCurrentUTCOffset description (at top
of page): insert "the" before "UTC".

Page 49, ptpbaseClockTransDefaultDSDelay description (at top of page):
change "shall be" to "of".  Change "If" to "if".

Page 56, ptpbaseClockPortRunningTable description: should that be “a
dataset" or "datasets".  Plain "dataset" doesn't work.

Page 57, ptpbaseClockPortRunningEntry description: change "runing” to
"running" unless Viking writing is involved.

Page 58, ptpbaseClockPortRunningState description: you probably don't need
to include the states here since they are already found in the definition
of ClockPortState.

Page 60, ptpbaseClockPortRunningEncapsulationType description: change
"eg.” to "e.g.,".

Page 60, ptpbaseClockPortRunningRxMode description: change "specifie” to
"specifies".

Page 63, ptpbaseClockPortTransDSPeerMeanPathDelay description: delete the
parentheses and append a comma after PTP.  On the following page, change
"value is" to "value of".

Page 64, ptpbaseClockPortAssociateTable description: change "which” to
"that".

Page 75, ptpbaseMIBClockPortTransDSGroup description: does “ TransparentDS
Dataset" contain a redundancy?

Page 76, 4th paragraph, 1st sentence: delete "the" before "implementers”.
Delete "as".

Page 76, 5th paragraph, 1st sentence: change "Further" to "Furthermore".

---

From: vinays <[email-masked]>
Date: Wed, 20 Apr 2016 22:41:34 -0400

Have addressed the MIB related comments below. The remaining will be 
addressed
by Tim in the latest revision to be submitted tomorrow morning UK time.

Please see inline at VC.

> Replace all occurrences of "create logical group" with "create a logical

VC: done.

> support" to "MIB supports".
VC: done.

> Page 8, Acronyms, EUI: delete the period after the acronym expansion.

VC: done.

> Page 12, PTP node definition: insert "A" before the sentence.

VC: done.

> synonym for EUI-48, so perhaps it should be defined.

VC: done. Have added MAC-48 in the acronym section.

> "end-to-end".  Replace "peer to peer" with "peer-to-peer".

VC: done.

> "Transport" to "transport".

VC: done.

> "an" before "address".  Change "and" to "or".

VC: done.

> square brackets.  Change "to start with" to "starting at".

VC: done.

> Page 23, ClockStateType description: insert "a" before "PTP".

VC: done.

---

From: vinays <[email-masked]>
Date: Wed, 20 Apr 2016 22:44:00 -0400

Sent too soon. Please see inline at VC2.

Thanks,
-Vinay.

>> Page 24, Holdover state text: change "ptp" to "PTP.  Change "FREERUN” to

VC2: done.

>> Change "to start with" to "starting at".

VC2: done.

>> Page 26, ClockTimeSourceType reference: change "Section" to "Sections".

VC2: done.

>> ptpbaseClockCurrentDSMeanPathDelay references: the indentation looks 

VC2: done.

>> Page 32, ptpbaseClockCurrentDSMeanPathDelay description: change 

VC2: done.

>> Page 35, ptpbaseClockParentDSOffset description (near top of page): 
VC2: done.

>> Page 35, ptpbaseClockParentDSClockPhChRate description: change 

VC2: done.

>> Append a comma after "engine".

VC2: done.

>> here, why not merely point back to ClockStateType?  Btw, PTP engine 

VC2: done.

>> Dataset" contain a redundancy?

VC2: All the above are done as well. Thanks,
-Vinay.

---

From: Jari Arkko <[email-masked]>
Date: Thu, 21 Apr 2016 07:03:52 +0200

Thanks for the review Peter, and for the updates, Vinay!

I do have a remaining question about the copyright. Do we have an answer?

Jari

On 21 Apr 2016, at 04:44, vinays <[email-masked]> wrote:

>> Have addressed the MIB related comments below. The remaining will be addressed
> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art