Hi, Thanks for this detailed review, it is much appreciated! Comments inline.
Juergen Schoenwaelder <[email protected]> wrote: > Hi, > > I have read through draft-ietf-netmod-rfc6020bis-07 (except section 9. > Built-In Types). Overall, the document is in a good shape. I spotted a > number of editorial issues or a number of issues where we could try to > seek a clearer separation of NETCONF specifics. (I plan to read > section 9 as well but I assume this is less important since there > likely are not many changes from RFC 6020 in this part.) > > I am indicating the paper number below for each comment/suggestion. > > * p8: > > s/XML has been/XML have been/ > > s/how the data model/how a data model/ > > s/represented in/encoded in/ Fixed. > * p9: > > s/names means/names mean/ Fixed. > * p11: > > s/the module faithfully/a module faithfully/ Fixed. > I am also wondering why we use device and server. It seems we use > these terms interchangeably. If so, for clarity, I would suggest to > use a single term, that is s/device/server Ok, fixed. > / and perhaps explicitly > state that unless stated otherwise server means a server providing > access to a YANG defined data tree. Yes this makes sense. But then I guess we shouldn't import client and server from 6241. (And most other documents (restconf etc) should probably import these terms from ths document). See also below. > * p12/p13 > > We import 7 terms from RFC 6241. Would it make sense to copy the > necessary text in order to avoid a too strict binding to RFC 6241? > In particular, 'client' and 'server' means NETCONF client and server > if we import from RFC 6241 but this may be a bit narrow given that > we have RESTCONF as well. In an ideal world, we would factor out > core architectural concepts but the best we can do is likely to > define core concepts inline, pointing out where they are the same. > The idea is to loosen the coupling to RFC 6241. Example: > > OLD > > o datastore: an instantiated data tree > > NEW > > o datastore: A conceptual place to store and access information. > A datastore might be implemented, for example, using files, a > database, flash memory locations, or combinations thereof. > [Matches the definition in RFC 6241.] To start with, I think we should define client and server more generically than just NETCONF: server: An entity that provides access to YANG-defined data to a client, over some network management protocol. client: An entity that can access YANG-defined data on a server, over some network management protocol. ? > * p13 > > Does the term 'mandatory node' really deserve its own subsection? I > understand that there are references to section 3.1 but would > reference to section 3 not work as well? Sure. The thing is that I have toolchain problems with nested lists... [hacking away] ... Fixed. > * p13 > > s/used for other/used with other/ Fixed. > * p14 > > s/encoded in NETCONF operations/encoded on-the-wire/ Fixed. > * p14 > > s/of NETCONF data models/of data models for network management > protocols such as NETCONF,/ Fixed. > And I suggest to remove the following sentence since it is not > really needed. > > The data models described by YANG are designed to be easily > operated upon by NETCONF operations. Fixed. > * p15 > > s/read-only access/read-only access [RFC6643]/ Fixed. > * p15 > > I am not sure why the last paragraph in section 4.1 is needed. In > the first setence, I would remove 'Like NETCONF' and in the second > sentence I am not sure what we are saying given that there is NACM. > I would definitely remove the second sentence and if the first > should be kept, I would likely move it up since it is a fairly > general statement. But I think removing the whole paragraph is the > simplest solution since it is not essential. Agree, removed. > * p13 > > Would it make sense to add a sentence right at the beginning of > section 4 stating that section 4 is intended to provide orientation > for the first time reader but that section 4 is not normative? How about: This non-normative section is intended to give a high-level overview of YANG to first-time readers. > * p15 > > With the previous in mind, I suggest to remove "This progressive > approach handles the inter-related nature of YANG concepts and > statements. A detailed description of YANG statements and syntax > begins in Section 7." from the text right below 4.2. Note that > Section 7 is actually Section 6 (but Section 5 has also important > normative content). I agree; removed. > * p16 > > s/four types of nodes/four types of data nodes/ Fixed. > * p16 > > Perhaps simplify: > > OLD > > A leaf instance contains simple data like an integer or a string. It > has exactly one value of a particular type and no child nodes. > > NEW > > A leaf data node has exactly one value of a particular type and no > child nodes. We changed this to "leaf instance" after discussion with Lada. I think it makes sense to mention that it is supposed to contain data like an integer or string, so I think I prefer to keep this text as is. > * p16-p25 > > Should this be 'XML Encoding Example' instead "NETCONF XML Example'? > This does not seem to be NETCONF specific. Fixed. > * p25 > > s/XML representation/XML encoding/ Fixed. > * p25 > > s/operations' names/operations' name/ Fixed. > * p25 > > s/node in the data hierarchy/data node/ Fixed. > * p28 > > Start section 5 by saying that this section defined language > concepts and that it is normative, e.g. > > This normative section defines core language concepts. But aren't all sections normative unless we say so? Otherwise it seems we should start every section by saying that it is either normative or not. > * p29 > > s/its contents are unchanged/its content is unchanged/ I am not sure that this is correct. The text is present in RFC 6020, and has been reviewd by the RFC editor; they usually capture things like this. > * p37 > > s/following shows valid data/following XML encoding shows valid data/ Fixed (I did "following XML encoding example...") > * p35 > > I think there should be a citation of I-D.ietf-netconf-yang-library > where this module first appears. Fixed. > * p46 > > The text concerning module names is a repetition from section 5.1. > To avoid introducing inconsistencies, perhaps replace the repeated > text with a reference to section 5.1? Ok, fixed. > * p49 > > Is the text in section 7.1.3 correct when it says all identifiers > defined by the module? I mean, schema node identifiers are > namespaced by module names and their prefixes. And data node > identifiers are using the namespace in the XML encoding. Here is > an attempt of a rewrite: > > The "namespace" statement defines the XML namespace that all data > node identifiers defined by the module are qualified by in the > XML encoding. Data node identifiers defined inside a grouping are > an exception q(see Section 7.13 for details). The argument to > the "namespace" statement is the URI of the namespace. It's actually not only data nodes, it is also rpc, actions, notifications, identities. How about: The "namespace" statement defines the XML namespace that all identifiers defined by the module are qualified by in the XML encoding, with the exception of identifiers for data nodes, action nodes, and notification nodes defined inside a grouping (see ^uses^ for details). The argument to the "namespace" statement is the URI of the namespace. > * p53 > > Another repetition of the module/submodule name requirements, does > it make sense to avoid repetition? Yes, fixed. > * p61 > > Should the text in 7.5.4.1 and 7.5.4.2 say explicitly that we talk > about NETCONF when we refer to <rpc-error>? Or should the text try > to be more generic, saying that the respective fields will be > carried in error message in protocols that use YANG? It is actually > somewhat opaque what an error-app-tag is or how it should be used. I think we can say "If the constraint evaluates to false, the string is passed as <error-message> in the <rpc-error> in NETCONF." Other protocols have to define how these fields are used. > * p63 > > Should the last paragraph in 7.5.7 be factored out into its own > subsection titled "NETCONF <get> and <get-config> Operations"? The > text is not really anymore about XML encoding (which may be used > with RESTCONF). Or the text is actually about the encoding but > should be written to simply state that non-presence containers can > be pruned in encodings (without restricting this to NETCONF). This makes sense. I think the last paragraph can be replaced with: If a non-presence container does not have any child nodes, the container may or may not be present in the XML encoding. > * p67 > > Similar to the comment for p63. Perhaps the right way is to phrase > this in such a way that is simply says leaf nodes containing a > default value may not be present in the XML encoding. Simply remove > NETCONF <get> or <get-config> specifics. Or maybe we should simply remove the last paragraph completely? For NETCONF, RFC 6243 details how leafs with defaults are handled. > * p72 > > s/an unspecified order/an order determined by the system/ > > Note that a description clause may actually define how the system > has to order elements and hence 'unspecified order' seems wrong. Fixed. > * p97 > > The text in 7.14 talks about NETCONF specifics, namely the <rpc> > element and the "rpcOperation" substitution group. Perhaps move this > down into a specific section defining the mapping of YANG RPCs to > NETCONF <rpc>s. Yes, fixed, but see below. > * p100 > > The XML encoding text starts with detailing NETCONF specifics. > Would it make sense to separate the XML encoding of the rpc and its > input/output from the specifics how the RPCs fit into NETCONF's > <rpc> system? Hmm. NETCONF and RESTCONF use quite different encodings of rpcs and their output. NETCONF: <rpc message-id="101" xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> <rock-the-house xmlns="http://example.net/rock";> <zip-code>27606-0100</zip-code> </rock-the-house> </rpc> <rpc-reply message-id="101" xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> <ok/> </rpc-reply> RESTCONF: POST to .../rock-the-house <input xmlns="http://example.net/rock";> <zip-code>27606-0100</zip-code> </input> result: 204 no content Maybe it would have been better to have a common encoding, like: <rock-the-house xmlns="http://example.net/rock";> <input>...</input> </rock-the-house> and <rock-the-house xmlns="http://example.net/rock";> <output>...</output> </rock-the-house> but this cannot be done now. So, maybe the simplest solution would be to rename the section to "NETCONF XML Encoding"? > * p102 > > The XML encoding section again mixes XML encoding of the node > hierarchy with NETCONF specifics how an action is invoked using > NETCONF's <rpc> system. I suggest to factor this into two different > sections. See above, I suggest we simply rename the section to "NETCONF XML Encoding". > * p105 > > Again, the proposal is to separate the XML encoding of notifications > from the details how notifications work with RFC 5277. Also notifications are encoded differently in RESTCONF and NETCONF. > * p120 > > The text in section 7.21.1 talks about NETCONF specific operations. > Is this actually necessary? Can the purpose of the config statement > not be described by referring to general concepts such as > configuration datastores instead of using protocol specific > operations? Yes, maybe we can just say: Data nodes representing configuration are part of configuration datastores. > * p123 > > Is there a special reason why the text uses 'notification instance > data tree' instead of just 'notification data tree'? No, fixed. > * p123 > > "All leaf data values MUST match the type constraints..." may be > read as 'all data nodes that contain values' or 'all instantiations > of nodes defined by the leaf statement'. I don't really see what you mean. Can you suggest new text? > * p192 > > The Acknowledgements section may need some updates. Ok. /martin _______________________________________________ netmod mailing list [email protected] https://www.ietf.org/mailman/listinfo/netmod
