Re: [core] Robert Wilton's Discuss on draft-ietf-core-yang-cbor-16: (with DISCUSS and COMMENT)

[Carsten Bormann <[e-mail]>]

Hi Rob,

here is finally the response to your feedback, based on yang-cbor-18:

https://www.ietf.org/rfcdiff?url2=draft-ietf-core-yang-cbor-17.txt
https://www.ietf.org/rfcdiff?url2=draft-ietf-core-yang-cbor-18.txt

> On 2021-07-15, at 15:35, Robert Wilton via Datatracker <[e-mail]> wrote:
> 
> 
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Thanks for this document, it is good work, and I think that there specification
> is almost there, but that the text could be tightened up in a few places.
> 
> 1. The document should be clearer on its use of terminology around schema
> nodes.  Mostly the encoding related to YANG data nodes, not YANG schema nodes. 
> I've provided more information in the comments section.

Right.
Neither schema node nor data node are the term we need most.
We have introduced the term “representation node” for instances of schema nodes that turn up in representations.

> 2. As also raised by Ben, this document should probably cover the YANG data
> structure extension in RFC 8791.  This could potentially be done in addition to
> rc:yang-data, but perhaps better in its place.

For the internal usage of YANG in the two drafts:
We changed the ietf-sid-files module to use sx:structure.
https://github.com/core-wg/yang-cbor/pull/116
This is about the documents’s *use* of rc:yang—data migrating to sx:structure.

As far as showing sx:structure examples with encoding from YANG-CBOR:
We have introduced sx:structure as another representation node, equivalent with container.
This should cover all aspects of representing sx:structure.

We do not plan to remove the support for rc:yang-data.

> 3. Did the WG consider supporting encoding YANG metadata (RFC 7952)? 
> Presumably this would be expected to be covered as future work?

The design team did; we did not discuss this on the WG list.
Supporting RFC 7952 metadata is a non-trivial effort, and we wouldn’t want that as a late addition to the present document.
We do plan to set up another document that defines the CBOR representation of metadata, but not to include this in the present document (or round of documents).

> 4. How does the CBOR encoding of SIDs apply to YANG features?  This draft
> references features and the SID draft allows SIDs for them, but I don't
> understand how they are used in the encoding (since features don't appear in
> the instance data, they are only at the schema level).

YANG features can occur in the data of a YANG module, e.g., yang-library (RFC 8525).  Using a SID instead of an identifier for a feature allows a compact representation.

> 5. I also support Ben's second discuss point.  I think that as written, this
> draft needs a normative reference to the SID draft.

I don’t necessarily agree, but this is an easy change without negative consequences.

> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> 1. Abstract:
> This document defines encoding rules for serializing configuration
>  data, state data, RPC input and RPC output, action input, action
>  output, notifications and the yang-data extension defined within YANG
>  modules using the Concise Binary Object Representation (CBOR, RFC
>  8949).
> 
> When I read the abstract, it raises the question about what is left out. 
> Hence, I am wondering whether it wouldn't be better to just describe it as
> encoding YANG data tree instance data.  However, I see that the description
> effectively mirrors the abstract for the YANG JSON encoding (RFC 7951), so it
> is at least consistent.

We discussed this and do want to stay consistent with RFC 7951.

> 2. I find this document (along with the SID draft), somewhat confusing in that
> it references YANG schema nodes, but in most cases, it probably only cares
> about the subset of YAND schema nodes that represent itemsthat are present in
> data tree instantiation.  RFC 7950 calls such 'schema nodes' as 'data nodes'. 
> For example, 'choice', 'case', 'input' and 'output' exist as nodes in the YANG
> schema tree, but are not data nodes and don't appear as instantiated data nodes.

The representation may also include information about schema nodes for action, rpc, input, output, and notification.  We introduced a new term, representation node, for this subset of schema nodes (see above).

> Where this causes problems are in the definitions of: 'child' - (e.g., 'case'
> cannot be a child of a 'container' only a child of a 'choice', and equivalently
> for the definition of 'parent'.  This definitions are not heavily used and
> could perhaps be removed?

We removed the definition of “child”, as it was used in only one place and that could easily be substituted by “schema node defined within”.
We left the term “parent”, as this is used seven times, and clarified it to mean The schema node of the closest enclosing representation node.

> Hence my suggestion to clarify the text are:
> - potentially import and use data node rather than schema node.  Note that
> data node, as defined in RFC 7950, is a subset of schema nodes, so you still
> need the text saying an instance of a data node.  

We do import data node and already used it occasionally in -16.
By the way, in the whole of YANG, I can only find RFC 7952 that ever talks about an “instance of a data node”, so I’m a bit reluctant to use that terminology throughout — any problem with using “data tree node”?

> Arguably, the RFC 7950
> definition of a data node is somewhat confusing. - please check everywhere
> where 'schema node' or 'data node' turns up in the text to ensure that you are
> referencing instances of that schema node rather than the schema node itself. 
> E.g., the second paragraph in section 3 states 'where each child schema node
> is encoded ...', but this should be an instance of child schema node. - ensure
> that if you are referring to the parent or child of a 'schema node' that the
> logic skip out the schema nodes that don't get encoded in the data tree. 
> E.g., when calculating SIDs.

This was covered by introducing the term “representation node” discussed above; please see updated introduction to Section 3.

> 3. I think that some of the references to submodules are not quite right. 
> Basically, the CBOR encoding should not need to concern itself about submodules
> at all, since logically, it works with the module schema (which logically
> incorporates the submodules).  E.g., perhaps you could mention this in the
> introduction, referencing section 4.2.1 (4th paragraph in particular) of RFC
> 7950?
> 
> Hence:
> In section 2:
>  *  item: A schema node, an identity, a module, a submodule, or a
>     feature defined using the YANG modeling language.
> 
> I think that this should exclude submodule (and possibly feature as well).

Submodule is mostly gone from -18, except for paragraphs such as:

>> Note that any structuring of modules into submodules is transparent to YANG-CBOR:
>> SIDs are not allocated for the names of submodules, and any
>> items within a submodule are effectively allocated SIDs as part of
>> processing the module that includes them.


> In section 3.2:
>  YANG modules, submodules, and features
> 
> I don't think that you want submodules in this list (and perhaps not features
> either).

Indeed, we removed all instances of “submodule” in lists of nodes.
(See also reply to “features” above.)

> And in:
>       6.13.1.  SIDs as instance-identifier
> 
>          SIDs uniquely identify a schema node.  In the case of a single
>          instance schema node, i.e., a schema node defined at the root of a
>          YANG module or submodule or schema nodes defined within a container,
>          the SID is sufficient to identify this instance.
> 
> I would remove "or submodule" from this text.  Effectively, the text about
> modules already covers this.

(See above.)

> 4. Please check the text that describes how lists are encoded.  Section 4.2
> seems to suggest that they are encoded as a CBOR Map, section 4.4 states that
> they are encoded as an array.  I presume that the answer is that they encode
> the list is an array, and each list entry is a Map.

We now consistently use “list entry” (Section 7.8 of RFC 7950) for representation nodes that form a list.

> 5.
>  Values of 'bits' types defined in a 'union' type MUST be encoded
>  using a CBOR text string data item (major type 3) and MUST contain a
>  space-separated sequence of names of 'bits' that are set
> 
> It might be helpful to have a quick sentence to justify why this is done.

We added references to the section 6.12 in 6.6 and 6.7.
We added some explanation in 6.12.
Fully explaining the unfortunate idiosyncrasy is a bit beyond the scope of this specification, though.

Grüße, Carsten