Re: [core] Review of draft-ietf-core-yang-cbor-15

[Ivaylo Petrov <[e-mail]>]

Hello Francesca,

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 Wed, Feb 24, 2021 at 1:41 PM Francesca Palombini
<francesca.palombini=[e-mail]> wrote:
>
>  Thanks for the work on this document. I have reviewed it and have some comments. Please handle these comments together with the Last Call comments.
>
> Chairs: I believe it would make sense to request a review from YANG Doctors for this document. Let me know if you need help setting that up.
>
> Thanks,
> Francesca
>
> ==
>
> 4.4.  The 'list' and 'list' instance(s) - should this second 'list' be "subset of 'list'"

[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.6:
>
> anyxml value MAY
>    contain CBOR data items tagged with one of the tags listed in
>    Section 9.3, these tags shall be supported.
>
> Why not BCP 14 SHALL?

[IP]: For certain types of data we can know from the CBOR type what is
inside the anyxml (true/false/null, etc). For others, that is not the
case and that's where we need the tags.

> ==
>
> * General question: about the SID values used throughout the document: as far as I can tell these are not allocated right now (and they will take some time to get registered as defined in core-sid document) It might be good to add a sentence about where the SID values from the examples come from.

[IP]: I believe most/all of those values can be considered fictional
and provided only as examples, hence the need to provide the element
being encoded each time. I believe this makes reading the text also
easier as you are not expected to remember information for too long. I
will ask my co-authors if there is specific reasoning behind the
values and add that sentence.

> ==
>
> Section 6.6 (and other sections that use tags defined by this document):
>
> There is a new CBOR tag appearing in this section's example, tag 44. This is quite surprising without any text. Although the registration follows the template from RFC8949, I would have expected to see some descriptive text about the tag (and not only appearing in the sentence "The encoding MUST be enclosed by the enumeration CBOR tag as specified in Section 9.3." - note that section 9.3 does not specify how to use the tags).
> This is valid for the following sections specifying new CBOR tags as well.

[IP]: I am not certain what kind of additional text would be useful
for the reader. Is the fact that inside tag 44 there needs to be an
encoding of an enum not clear or is it rather why is that tag
necessary. Or maybe something else?

> ==
>
> Section 6.7:
>
> I find it hard to understand the example, without any indication on which bits are set.

[IP]: Modified

    The following example shows the encoding of an 'alarm-state' leaf
instance with
    the 'critical', 'warning' and 'indeterminate' flags set.

to

    The following example shows the encoding of an 'alarm-state' leaf
instance with
    the 'critical' (position 3), 'warning' (position 8) and
'indeterminate' (position 128) flags set.

> ==
>
> Nits
>
> - expand acronyms on first use ( e.g. NETCONF, RPC, see https://www.rfc-editor.org/materials/abbrev.expansion.txt )

[IP]: Done for acronyms that I noticed.

> - move YANG SID terminology description before delta description.

[IP]: Done.

> Section 4.5
>
>    module event-log {
>      ...
>      anydata last-event;                # SID 60123
>
> Please close the parenthesis of this example

[IP]: Done.

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