Hi Med, On Thu, Dec 19, 2024 at 7:50 AM <mohamed.boucadair@xxxxxxxxxx> wrote: > > Hi Per, > > Thank you for the review. > > The changes to address your comments can be tracked at: https://author-tools.ietf.org/api/iddiff?url_1=https://netmod-wg.github.io/enhanced-acl-netmod/draft-ietf-netmod-acl-extensions.txt&url_2=https://netmod-wg.github.io/enhanced-acl-netmod/Per-review/draft-ietf-netmod-acl-extensions.txt. These will be published in -13. Thanks! > Please see inline. See my responses inline below. > Cheers, > Med > > > -----Message d'origine----- > > De : Per Andersson via Datatracker <noreply@xxxxxxxx> > > Envoyé : jeudi 19 décembre 2024 04:13 > > À : yang-doctors@xxxxxxxx > > Cc : draft-ietf-netmod-acl-extensions.all@xxxxxxxx; last- > > call@xxxxxxxx; netmod@xxxxxxxx > > Objet : Yangdoctors last call review of draft-ietf-netmod-acl- > > extensions-12 > > > > > > 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 > > > > [Med] Hmm. I confirm that all the following are already cited in the document: > > * 9293, 4443, 3032, 0792, IEEE802.1Qcp, and IEEE-802-1ah They are indeed listed in the references, but the comment asks to list them in the first paragraph in Section 4. Currently this is the only preamble This model imports types from [RFC6991], [RFC8519], and [RFC8294]. It is helpful for me to have all references briefly listed up front. But I'll leave it to you to decide. > > A general comment when using XSLT to create the initial IANA > > modules. > > The supplied XSLT stylesheets use > > <https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F% > > 2Fgithub.com%2Fllhotka%2Fiana- > > yang&data=05%7C02%7Cmohamed.boucadair%40orange.com%7Ce207c4322dd9 > > 4161486d08dd1fdb1cc3%7C90c7a20af34b40bfbc48b9253b6f5d20%7C0%7C0%7 > > C638701748127968303%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd > > WUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ > > %3D%3D%7C0%7C%7C%7C&sdata=YbVnlLmV3VvDIk7IuPi7U2QaOU0Ms%2BYrp8ZfR > > ZA6XhA%3D&reserved=0>. > > 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. > > > > [Med] This is mentioned several times in the draft: e.g., > > In the ACK: > > The IANA-maintained modules were generated using an XSLT stylesheet > from the 'iana-yang' project (https://github.com/llhotka/iana-yang). > > In the modules: > This version of this YANG module was generated from the > corresponding IANA registry using an XSLT stylesheet from the > 'iana-yang' project (https://github.com/llhotka/iana-yang)."; > > Added a new statement to in the introduction. Thank you. I managed to miss where it was already mentioned. > > 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. > > [Med] Work for me. Went for length and pattern. Thanks. Thanks! > > iana-icmpv4-types@xxxxxxxxxxxxxxx: > > > > * pyang fails with > > > > iana-icmpv4-types@xxxxxxxxxxxxxxx:280: error: unterminated > > statement > > definition for keyword "enum", looking at I > > > > [Med] This is because of folding. The module will be removed anyway from the final document. As long as the final YANG module validates accord to our rules. > > * 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. > > Med: The registry is authoritative here. We are mirroring exactly what is in the registry. Won't make this modification. > > 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. > > Med: RFC2780 is not useful for understanding these types. I don't think we need to touch this, but this is beyond the scope of this doc. Ok. Perhaps a good idea to notify IANA anyway. I can do that, any pointers for whom to contact is greatly appreciated! > > > > 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. > > Med: OK This would also resolve the pyang and yanglint errors. > > 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@2023- > > 04-28.yang \ > > > iana-icmpv6-types@xxxxxxxxxxxxxxxxxxxx > > > > $ diff -u iana-icmpv6-types@xxxxxxxxxxxxxxx \ > > > iana-icmpv6-types@xxxxxxxxxxxxxxxxxxxx > > > > - enum > > ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby > > { > > + enum > > + > > + > > ICMPmessagesutilizedbyexperimentalmobilityprotocolssuchasSeamoby > > { The XSLT stylesheet change for icmpv4 is applicable for icmpv6 as well. Thank you for your contribution! -- Per -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
