Subject: Re: [Last-Call] [core] Yangdoctors last call review of draft-ietf-core-yang-cbor-15

From: Ivaylo Petrov <[email-masked]>
Date: Mon, 14 Jun 2021 13:46:59 +0200

Hello Joe,

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=draft-ietf-core-yang-cbor&url2=http://core-wg.github.io/yang-cbor/draft-ietf-core-yang-cbor-latest.txt
[2]: https://core-wg.github.io/yang-cbor/draft-ietf-core-yang-cbor-latest.txt
[3]:  https://core-wg.github.io/yang-cbor/draft-ietf-core-yang-cbor-latest.html

On Tue, Mar 16, 2021 at 8:37 PM Joe Clarke via Datatracker
<[email-masked]> wrote:
>
> Reviewer: Joe Clarke
> Review result: Almost Ready
>
> I have been asked to review this document on behalf of yang-doctors.  Overall,
> I found the document with its many examples to be quite readable and clear.
> However, I did find a few readable and typo issues and I have a few questions.
> See below.
>
> ===
>
> Abstract:
>
> s/notifications and yang-data/notifications and the yang-data/

[IP]: Fixed.

> ===
>
> Section 2:
>
> Move the definition of YANG Schema iDentifier higher up in the list of
> terminology so it's defined before you use it when discussing deltas.  This
> should likely be first in the list.

[IP]: Fixed.

> ===
>
> Section 3
>
> s/When schema node are serialized/When schema nodes are serialized/

[IP]: Fixed.

> ===
>
> Section 3.3
>
> s/identifiers as string, similar/identifiers as strings, similar/

[IP]: Fixed.

> ===
>
> Section 4.1
>
> In other examples you include the associated type definition inline.  You don't
> do that with inet:domain-name.  In fact, you _do_ include the domain-name
> typedef in Section 4.3, but I think it should move up here as well to aid in
> readability.

[IP]: Added the typedef here as well to make the example
self-contained and simpler for reading.

> ===
>
> Section 4.2.1
>
> s/In the case of an 'notification content'/In the case of a 'notification
> content'/

[IP]: Fixed.

> ===
>
> Section 4.2.2
>
> You duplicate the YANG snippet here that you included in the overarching
> Section 4.2.  I don't think both are needed.  Probably best to drop this here
> since you didn't include it in 4.2.1.

[IP]: Agreed! Fixed.

> ===
>
> Section 4.4
>
> You use the term "list instance" to mean what I think is better stated as "list
> entry".  The latter is clearer with respect to an element within a list vs. the
> instance of a list itself.  You use "list instance" in Section 3 as well (and
> in other places in the document) where I think "list entry" would be clearer.

[IP]: There were places where instances were used as instantiations of
schema nodes and places where instances were used instead of entries.
I tried to clear this out.

> ===
>
> Section 4.4.1
>
> I think documenting the true/false value of the primitives here (noted in the
> CBOR encoding) would aid in clarity.  For example, "primitive(20) [false]".

[IP]: I am not against that, I am only concerned if that would be
readable for others that are used to the diagnostic notation,
otherwise I am fine to apply this change.

> ===
>
> Section 4.5.1
>
> I'm being pedantic here, but ahead of the { 60123 : { ... example, you usually
> state "CBOR diagnostic output".  You don't here, though.  I think you should
> add it.

[IP]: I might be misunderstanding the point, but it appears to me that
there is such a note already, only it unfortunately appeared at the
top of the previous page in the txt version and was quite easy to
miss.

> ===
>
> Section 4.6
>
> Concerning text "anyxml value MAY contain CBOR data items tagged with one of
> the tags listed in Section 9.3, these tags shall be supported.":
>
> This sentence fragment makes no sense.  Did you mean something along the lines
> of: "the tags listed in Section 9.3 SHALL be supported"?

[IP]: I believe I fixed that now.

> ===
>
> Section 5
>
> s/Just like YANG containers, yang-data/Just like YANG containers, the yang-data/

[IP]: Fixed.

> ===
>
> Section 6.7
>
> s/same example yang definition, but this/same example YANG definition, but this/

[IP]: Fixed.

> s/byte string MUST instead by encoded/byte string MUST instead be encoded/

[IP]: Fixed.

> ===
>
> Section 6.13.1
>
> It isn't clear to me how a YANG list with multiple keys or a YANG list with no
> keys would be encoded in CBOR.  I think examples and some clarifying text would
> help.

[IP]: I have modified one of the examples so that it uses two keys. As
for the other point, Is it possible to have a list with no keys being
referenced through an instance-identifier? My understanding is that
this is not possible, but I might be wrong. If it is not possible, we
will only need to clarify this in the text. If it is possible, can we
use the position in the list to identify the element?

>
>
> _______________________________________________
> core mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/core

---

From: Carsten Bormann <[email-masked]>
Date: Mon, 14 Jun 2021 14:50:54 +0200

> On 2021-06-14, at 13:46, Ivaylo Petrov <[email-masked]> wrote:
> 

Let me pick this one because it is all my fault.

The “CBOR encoding” snippets are generated by the CBOR diagnostic tool (which is also available at cbor.me on the Web.)

The comments it generates in its “pretty hex” output are rather generic comments about the encoding; they are not trying to reflect the exact meaning of the content.  This usually comes up with negative numbers (where the argument numbers shown are not bit-wise inverted to generate the negative number), but more detail could also be added for false/null/true/undefined.

Instead of manually tweaking dozens of generated examples, I think it would be a good idea to agree about the best way to update the tool.  Until that is done and implemented, I would feel uncomfortable with all this tweaking, because it will be error-prone and inconsistent.

Grüße, Carsten

---

From: "Joe Clarke (jclarke)" <[email-masked]>
Date: Mon, 14 Jun 2021 16:28:20 +0000

> [3]:  https://core-wg.github.io/yang-cbor/draft-ietf-core-yang-cbor-latest.html

Thanks, Ivaylo.  I'll look through the diffs, but it sounds like you
addressed all of my comments.

> otherwise I am fine to apply this change.

Sounds like Carsten addressed this in a follow-up.  I think his response
is reasonable whereby manual effort is removed in favor of an update
toolchain.

> top of the previous page in the txt version and was quite easy to

Yep.  Sorry.  Those darn page breaks...

> use the position in the list to identify the element?

I'm referring to something like the last example in Section 9.13.4 of
RFC 7950 for an i-i for a list entry without keys.  In this case, a
numeric identifier is used.

Joe