Re: [core] Yangdoctors last call review of draft-ietf-core-sid-15

[Xufeng Liu <[e-mail]>]

Hi Ivaylo,

Thanks for addressing the comments. One more minor comment on item 1.

Best regards,
- Xufeng

On Sun, Jun 13, 2021 at 5:52 PM Ivaylo Petrov <[e-mail]> wrote:

> Hello Xufeng,
>
> Thank you for your review and apologies for the delay! Please find our
> answers to your questions below. The diff with -15 is available here
> [1]. The updated version is available as txt [2] and as html [3].
>
> Thanks,
> Ivaylo
>
> [1]:
> https://tools.ietf.org/rfcdiff?url1=https://tools.ietf.org/id/draft-ietf-core-sid-15.txt&url2=https://core-wg.github.io/yang-cbor/draft-ietf-core-sid-latest.txt
> [2]: https://core-wg.github.io/yang-cbor/draft-ietf-core-sid-latest.txt
> [3]:  https://core-wg.github.io/yang-cbor/draft-ietf-core-sid-latest.html
>
> On Fri, Mar 26, 2021 at 12:40 AM Xufeng Liu via Datatracker
> <[e-mail]> wrote:
> >
> > Reviewer: Xufeng Liu
> > Review result: Ready with Nits
> >
> > This is a review of the YANG module in draft-ietf-core-sid-15.
> >
> > Minor Issues, Nits, and Questions:
> >
> > 1) This module uses the yang-data extension in RFC8040, which was the
> best
> > choice a few years ago when this draft started. However, RFC8791 has been
> > published so that the YANG structure extension is available now. Has the
> YANG
> > structure extension been considered to replace the yang-data extension?
> >
>
> [IP]: Thank you for providing this pointer! We would really prefer not
> to change this given the mature state of this document, unless there
> are some really important advantages of using YANG structure
> extension. Quickly looking at the document I did not find compelling
> reasons to do the change, but please do let us know if you believe
> there are such reasons.
>
[Xufeng]: If yang-data is kept,  Sec 2 of RFC8340 specifies the format for
the tree diagram for yang-data. In this case "yang-data" is used, so the
tree would be:

module: ietf-sid-file
  yang-data sid-file:
    +-- sid-file
      +-- module-name?              yang:yang-identifier


> > 2) The container sid-file is missing in the tree diagram in Section 4.
> RFC8340
> > specifies the tree format to represent such a yang-data definition. If
> the YANG
> > structure extension in RFC8791 is used, RFC8791 describes how the tree
> diagram
> > looks like for such a YANG structure. Also, the top container would not
> be
> > needed, because a YANG structure is encoded as a 'container'.
> >
>
> [IP]: I fixed the tree diagram. About adding the sid-file container,
> it was suggested in the following email:
> https://mailarchive.ietf.org/arch/msg/core/-1YKD5X75p9CeTIm1yt0BM3KtOg/.
> We can discuss with Andy, but from the email I am under the impression
> that he believed the container is needed.
>
[Xufeng] You are right the the container is needed for yang-data. If the
YANG structure extension in RFC8791 were used, the container would not be
needed.

>
> > 3) In the container sid-file, the leaf module-name is optional. What is
> the
> > assumption when it is not specified? It would be beneficial to clarify
> in the
> > description statement.
>
> [IP]: I could not remember why that was made optional. I will have to
> check with my co-authors and come back to you.
>
> > 4) In the container sid-file, the leaf sid-file-version is optional. The
> > description says that this number starts at zero. Let’s say that there
> are two
> > .sid files, one of which does not have the version number and the other
> one has
> > version number 0. Are they the same? If so, would it be better to have a
> > default statement with a default value of 0?
>
> [IP]: Fixed.
>
> > 5) For “list dependency-revision”, the key is module-name. The
> “mandatory
> > true” statement is not necessary for this leaf because it is a key.
>
> [IP]: Fixed.
>
> > 6) Under the “list dependency-revision”, the leaf revision-identifier is
> > specified as “mandatory”. What would this leaf be specified when a
> dependent
> > module does not have a revision?
>
> [IP]: Is it possible to make two different versions of a YANG module
> without revisions in practice? If the answer is no, then I believe we
> can make that optional. My understanding is that the answer would be
> maybe and then I do not think we can support this as it will add non
> determinism in the SID allocation, which we can not allow.
>
> > 7) “list assigment-ranges” should be “list assignment-ranges”. A letter
> ‘n’ is
> > missing in the current YANG module because of a typo.
> >
> > 8) For “list assignment-ranges”, the key is entry-point. The  “mandatory
> true”
> > statement is not necessary for this leaf because it is a key.
>
> [IP]: Fixed.
>
> > 9) As a convention, the node names of “list assignment-ranges” and “list
> items”
> > should be in the singular form, the same way as “list
> dependency-revision”, so
> > that they would be “list assignment-range” and “list item”.
>
> [IP]: Fixed.
>
> > 10) Since a YANG SID value is globally unique, would it be beneficial to
> have a
> > statement in the YANG file to describe the requirement formally? This
> can be
> > easily done by adding the following statement under “list items”:
> >         unique "sid";
>
> [IP]: Added.
>
> > 11) The format of the YANG file needs to be adjusted. Some of the lines
> in the
> > .yang file are longer than 69 characters. For example, at line 108 is:
> >        o  Any subsequent schema node name is in the namespace-qualified
> form
> > To examine, the command “pyang --ietf --max-line-length 69 FILE” can be
> used.
> > Before publishing, an RFC editor would normalize the format by using the
> > command “pyang -f yang --keep-comments --yang-line-length 69 FILE”. It
> would be
> > helpful to run this command now since it may change the lines to be
> longer than
> > the limit of 69 characters.
>
> [IP]: Fixed.
>
> > 12) In the example in Appendix A, the four "module-revision" statements
> contain
> > “.yang” after the date, not following the pattern rule of the
> > revision-identifier. It seems that the sid generation tool did not take
> out the
> > extension “.yang”.
>
> [IP]: Removed those and made a note to check the tool.
>
> > 13) In Appendix A, the 5th item is:
> >  o  [e-mail] (defined in [RFC7317])
> > but in  RFC7317, the revision of  iana-crypt-hash is 2014-08-06
>
> [IP]: I believe I manually downloaded or created that file when I last
> generated this and probably I have mistakenly put an incorrect
> revision. Now it's fixed.
>
> > 14) The following is a thought that may have been discussed before and
> decided
> > by the WG, but I’d mention it here anyway just in case: Since the scope
> of a
> > .sid file is for a single YANG file (module), many of the data nodes
> start with
> > the namespace of this particular module. The “schema-node-path” currently
> > requires an identifier string to always start with a namespace. Because
> of this
> > requirement, there are many repeated namespace strings in the
> identifiers of
> > the items. If we assume that the default starting namespace is the module
> > associated with the .sid file, we may shorten the .sid file?
>
> [IP]: On the constraint devices or over the network the SID file will
> not be present at all, therefore having some possibly redundant data
> is not a big issue and my concern is can we introduce possible
> ambiguity because of an attempt to save a few bytes where they were
> not creating an issue. If you feel strongly about this, we will bring
> it to the WG to discuss.
>
> > - Xufeng
> >
> >
> >
>