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

[Ivaylo Petrov <[e-mail]>]

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.

> 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.

> 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
>
>
>