Dale, thanks for your review. Xufeng, thanks for your response. I entered a No Objection ballot. Please disregard my ballot email — I missed this thread before balloting.
Alissa
Hi Dale,
Some details are in-line below.
Best regards, - Xufeng
Reviewer: Dale Worley
Review result: Ready with Nits
I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair. Please treat these comments just
like any other last call comments.
For more information, please see the FAQ at
<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
Document: draft-ietf-pim-igmp-mld-yang-10
Reviewer: Dale R. Worley
Review Date: 2019-02-05
IETF LC End Date: 2019-02-08
IESG Telechat date: not known
Summary:
This draft is basically ready for publication, but has nits
that should be fixed before publication.
I do not have the expertise to review the contents of the Yang
module itself. Fortunately, the Yang Doctor can review that.
Minor issues:
This draft has a number of exposition issues that should be fixed.
Abstract
This document defines a YANG data model that can be used to
configure and manage Internet Group Management Protocol (IGMP) and
Multicast Listener Discovery (MLD) devices.
Both here and in the Introduction, it would be better to say "devices
that implement IGMP and MLD" or something like that, since IGMP and
MLD are protocols, not classes of devices.
Table of Contents
2. Design of Data model.......................................... 4
2.1. Scope of model ........................................... 4
2.2. Optional capabilities .................................... 4
2.3. Position of address family in hierarchy .................. 5
3. Module Structure .............................................. 5
3.1. IGMP Configuration and Operational state ................. 5
3.2. MLD Configuration and Operational State .................. 8
It looks like the current style would capitalize "model",
"capabilities", "state", etc.
1. Introduction
This model will support
the core IGMP and MLD protocols, as well as many other features
mentioned in separate IGMP and MLD RFCs.
"will support" needs clarifying. Does the model defined by this
document "support many other features", or is this a prediction that
the model will eventually be extended to do so? Indeed, the
Introduction should make a clear statement of what is and is not
supported by this version of the model.
[Xufeng]:
Fixed. Should be present, not future.
1.3. Prefixes in Data Node Names
Otherwise,
names are prefixed using the standard prefix associated with the
The tail of this sentence is missing.
2. Design of Data model
2.1. Scope of model
The model covers IGMPv1 [RFC1112], IGMPv2 [RFC2236], IGMPv3
[RFC3376] and MLDv1 [RFC2710], MLDv2 [RFC3810].
This should be stated in the Introduction as well.
The configuration of IGMP and MLD features, and the operational
state fields and RPC definitions are not all included in this
document of the data model.
As written, this says that the model covers some unspecified subset of
IGMP and MLD features. Is it possible to characterize what is
included and what is not? Otherwise, the reader would have to work
through the model to check on every specific item they were interested
in.
[Xufeng]:
This section has been expanded to describe these features.
This model can be extended, though the
structure of what has been written may be taken as representative of
the structure of the whole model.
What does this mean? Like any Yang model, this model can be extended,
by anyone who chooses to do so. But how does "what has been written"
represent or constrain the structure of an extended model?
2.2. Optional capabilities
The main design goals of
this document are that any major now-existing implementation may be
said to support the basic model, [...]
Probably more correct to say "[...] may be said to support the
facilities described by the basic model [...]".
There is also value in widely-supported features being standardized,
to save work for individual vendors, [...]
And probably more importantly, so that the features can be accessed in
a standardized way.
[Xufeng]:
Added such a phrase.
2.3. Position of address family in hierarchy
The current document contains IGMP and MLD as separate schema
branches in the structure. The reason for this is to make it easier
for implementations which may optionally choose to support specific
address families. And the names of objects may be different between
the IPv4 (IGMP) and IPv6 (MLD) address families.
It seems like the qualification of IGMP == IPv4 and MLD == IPv6 should
be done in the first sentence rather than the last.
[Xufeng]:
Added clarification sentences at the beginning of this section.
3.1. IGMP Configuration and Operational state
It seems like this section has a first part which applies to IGMP and
MLD equally (though it only talks about IGMP), and a second part which
is a summary of the IGMP module. Perhaps they should be split into
two sections?
[Xufeng]:
Moved the paragraphs common to both IGMP and MLD to Sec 3. Put IGMP
specifics in Sec 3.1, and MLD specifics in Sec 3.2.
Interface-global: Only including configuration data nodes that
IGMP configuration attributes are applicable to all the interfaces
whose interface-level corresponding attributes are not existing,
with same attributes' value for these interfaces.
This sentence seems to have either extra words or missing words.
"SSM" seems to show up a lot but isn't defined. Is it part of IGMP/MLD?
[Xufeng]:
Added to Sec 1.1.
3.2. MLD Configuration and Operational State
3.3. IGMP and MLD RPC
IGMP and MLD RPC clears the specified IGMP and MLD group membership.
This is awkwardly phrased. Perhaps, "IGMP and MLD each have one RPC
which clears the group membership [database? table?] for that
protocol."
4. IGMP and MLD YANG Modules
The use of empty lines isn't consistent in the module definition.
[Xufeng]:
Went through the module and fixed them.
5. Security Considerations
These subtrees are all under
/rt:routing/rt:control-plane-protocols
/rt:control-plane-protocol/igmp:
Is the trailing "/igmp:" meaningful?
And parallel cases later in the section. As far as I can see, "igmp:"
etc. is part of the root node name of the subtree, not attached to
the path above it.
[Xufeng]:
The “igmp” is meant to indicate the node under
“control-plane-protocol”. The prefix “igmp:” before “global”
and “interfaces” is the name space, but it should be “igmp-mld:”
instead, because igmp and mld share the same module and name space.
Fixed these tokens in the document.
Unauthorized access to any data node of these subtrees can adversely
affect the membership records of multicast routing subsystem on the
local device.
-- and some similar cases. The scope of the phrase "these subtrees"
is unclear.
[Xufeng]:
The phrase “these subtrees” indicates all the trees listed above.
This paragraph is part of the template. Is it ok for us to modify
this paragraph?
6. IANA Considerations
This document registers the following namespace URIs in the IETF XML
registry [RFC3688]:
--------------------------------------------------------------------
URI: urn:ietf:params:xml:ns:yang:ietf-igmp-mld
Registrant Contact: The IESG.
XML: N/A, the requested URI is an XML namespace.
--------------------------------------------------------------------
RFC 3688 section 3.2 includes:
ns -- XML Namespaces [W3C.REC-xml-names] are named by a URI. [...]
Thus, the
registered document will be either the specification or a
reference to it. [...]
It seems to me that the "XML" field of this registration should be:
XML: RFC XXXX
to provide the name of the registered specification of the namespace.
[Xufeng]:
Section
14 in [RFC6020] registers two URIs for the YANG and YIN XML
namespaces in the IETF XML registry [RFC3688]. After it, all YANG
modules currently use this format.
_______________________________________________ Gen-art mailing list Gen-art@xxxxxxxxhttps://www.ietf.org/mailman/listinfo/gen-art
|