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

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 <vinays(_at_)cisco(_dot_)com> wrote:

> Sent too soon. Please see inline at VC2.
>
> Thanks,
> -Vinay.
>
>
> On 4/20/16 10:41 PM, vinays wrote:
> > 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.
> >
> >
> > On 4/17/16 4:18 PM, Peter Yee wrote:
> > > 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".
> > VC: done.
> >
> > > 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".
> > 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.
> >
> > > 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.
> > VC: done. Have added MAC-48 in the acronym section.
> >
> > > Page 17, ClockMechanismType description: replace "End to End” with
> > > "end-to-end".  Replace "peer to peer" with "peer-to-peer".
> > VC: done.
> >
> >
> > > Page 19, ClockPortTransportTypeAddress description, 2nd sentence: change
> > > "Transport" to "transport".
> > VC: done.
> >
> >
> > > Page 19, ClockPortTransportTypeAddress description, 4th sentence: insert
> > > "an" before "address".  Change "and" to "or".
> > VC: done.
> >
> >
> > > 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".
> > VC: done.
> >
> > > Page 23, ClockStateType description: insert "a" before "PTP".
> > VC: done.
> > > Page 24, Holdover state text: change "ptp" to "PTP.  Change "FREERUN” to
> > > "Freerun".
> VC2: done.
>
>
> > > 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".
> VC2: done.
>
> > > Page 26, ClockTimeSourceType reference: change "Section" to "Sections".
> VC2: done.
>
>
> > > Page 32, ptpbaseClockCurrentDSOffsetFromMaster and
> > > ptpbaseClockCurrentDSMeanPathDelay references: the indentation looks a bit
> > > off here.
> VC2: done.
>
>
> > > Page 32, ptpbaseClockCurrentDSMeanPathDelay description: change "measure”
> > > to "measured".
> VC2: done.
>
>
> > > Page 35, ptpbaseClockParentDSOffset description (near top of page): change
> > > "clocks" to "clock's".
> VC2: done.
>
> > > Page 35, ptpbaseClockParentDSClockPhChRate description: change "clocks” to
> > > "clock's".
> VC2: done.
>
>
> > > Page 41, ptpbaseClockRunningState description: insert "a" before "PTP”.
> > > Append a comma after "engine".
> VC2: done.
>
>
> > > 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.
> VC2: done.
>
> > > 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?
> VC2: All the above are done as well. Thanks,
> -Vinay.
>
>
>
> > > Page 76, 4th paragraph, 1st sentence: delete "the" before "implementers”.
> > > Delete "as".
> > >
> > > Page 76, 5th paragraph, 1st sentence: change "Further" to "Furthermore".
> _______________________________________________
> Gen-art mailing list
> Gen-art(_at_)ietf(_dot_)org
> https://www.ietf.org/mailman/listinfo/gen-art

signature.asc
Description: Message signed with OpenPGP using GPGMail