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. Please see inline. 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 > 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. > 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. > > 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; > [Med] ACK. Fixed. > 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. > * 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. > > 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 > > --- 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@2023- > 04-28.yang \ > > 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]. > Med: Good catches. Fixed. Thanks. > -- > Per > ____________________________________________________________________________________________________________ Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration, Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci. This message and its attachments may contain confidential or privileged information that may be protected by law; they should not be distributed, used or copied without authorisation. If you have received this email in error, please notify the sender and delete this message and its attachments. As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified. Thank you. -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
