[Last-Call] Yangdoctors last call review of draft-ietf-netmod-acl-extensions-12

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Reviewer: Per Andersson
Review result: Ready with Issues

Dear WG,

This is my Last Call review of draft-ietf-netmod-acl-extensions-12. My
conclusion is that the four modules are Ready with issues.

In general the modules are in good shape. The following issue from the
early yangdoctor review is still not addressed and should be fixed.

* RFCs cited in the YANG modules should be listed in the introductory
  section as references. Section 4 needs to add references to RFC 9293,
  4443, 3032, 792, and IEEE 802.1Q, IEEE 802.1ah

A general comment when using XSLT to create the initial IANA modules.
The supplied XSLT stylesheets use
<https://github.com/llhotka/iana-yang>.
I suggest this is mentioned because I did not know about this project
and had search for it myself by the import of iana-yinx.xsl.

Regarding match-on-payload, it is not understandable from Section 3.6
alone how the mechanism works. From my reading of the rest of the
document it works by matching the data after a given offset, with a
specific length, and computing the result of the operator applied to the
extracted payload and the binary pattern. I find the naming of
"offset-end" and "prefix" confusing, since "offset-end" is really a
length and not a position; suggest renaming it to e.g. "payload-length"
or "length". Regarding "prefix" I suggest renaming it to "pattern" or
"bitmask", since it might not be a prefix but an arbitrary part of a
payload and it is used for the matching.

ietf-acl-enh@xxxxxxxxxxxxxxx:

* pyang OK
* yanglint complains on imported module
  iana-icmp4-types@xxxxxxxxxxxxxxx..

Some identityrefs use prefix, some don't; suggest to be consistent and
add prefix to all. Especially since they are members of reusable
groupings.

L393: base tcp-flag;
L450: base label-positions;
L497: base offset-type;

iana-icmpv4-types@xxxxxxxxxxxxxxx:

* pyang fails with

    iana-icmpv4-types@xxxxxxxxxxxxxxx:280: error: unterminated statement
    definition for keyword "enum", looking at I

* yanglint validation fails:

    libyang err : Invalid character sequence
    "ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby",
    expected a keyword. (Line number 280.) libyang err : Parsing module
    "iana-icmpv4-types" failed. libyang err : Loading "iana-icmpv4-types"
    module failed. libyang err : Parsing module "ietf-acl-enh" failed.

Missing reference to RFC 2780 for Type 0, 3, 8, 9, 10, 11, 12, 13, 14,
and 40. I notice that there is a discrepancy in the IANA ICMP Parameters
registry listing of ICMP Type Numbers section and the Code Fields
section. Suggest to notify IANA and rebuild the YANG module.

The enum
ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby is
problematic because it is so long (when YANG modules should fit in 69
characters line length). This is why the tools can't validate the
module. Perhaps truncate it to
ICMPmessagesutilizedbyexperimentalmobilityprotocols?

Reading RFC 4065 it seems that ICMPExperimentalMobilityType is a fitting
name, since it is designed not only for Seamoby but also for other
experimental mobility protocols..

The following XSLT patch would remove "suchasSeamoby" from the enum
name.

--- iana-icmpv4-types.xsl.orig  2024-12-19 02:16:49.552187474 +0100
+++ iana-icmpv4-types.xsl       2024-12-19 02:16:01.684572966 +0100
@@ -63,8 +63,8 @@
                  '(Deprecated)')),' ','')"/>
          </when>
          <otherwise>
-           <value-of select="translate(normalize-space
-                   (iana:description),' ','')"/>
+           <value-of select="substring-before(translate(normalize-space
+                   (iana:description),' ',''),'suchasSeamoby')"/>
          </otherwise>
        </choose>
      </with-param>

iana-icmpv6-types@xxxxxxxxxxxxxxx:
* pyang OK
* yanglint OK

The following change is done when running

    $ pyang -f yang --yang-line-length=69 iana-icmpv6-types@xxxxxxxxxxxxxxx \
    > iana-icmpv6-types@xxxxxxxxxxxxxxxxxxxx

    $ diff -u iana-icmpv6-types@xxxxxxxxxxxxxxx \
    > iana-icmpv6-types@xxxxxxxxxxxxxxxxxxxx

-      enum ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby {
+      enum
+        ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby {

iana-ipv6-ext-types@xxxxxxxxxxxxxxx:
* pyang OK
* yanglint OK

Nits:

Quoting is wrong in Section 5

s/"iana-ipv6-ext-types defines"/"iana-ipv6-ext-types" defines/

Section 6.3.2 references [IANA-ICMPv4] when it should reference
[IANA-ICMPv6].

Section 6.3.3 references [IANA-ICMPv6] when it should reference
[IANA-IPv6].

--
Per


-- 
last-call mailing list -- last-call@xxxxxxxx
To unsubscribe send an email to last-call-leave@xxxxxxxx




[Index of Archives]     [IETF Annoucements]     [IETF]     [IP Storage]     [Yosemite News]     [Linux SCTP]     [Linux Newbies]     [Mhonarc]     [Fedora Users]

  Powered by Linux