Hi Joe, Thank you for the excellent review.
The changes to address the review can be seen here: https://author-tools.ietf.org/api/iddiff?url_1=https://boucadair.github.io/udp-ipfix/draft-ietf-opsawg-tsvwg-udp-ipfix.txt&url_2=https://boucadair.github.io/udp-ipfix/Joe-Review/draft-ietf-opsawg-tsvwg-udp-ipfix.txt Please see inline for more context. Cheers, Med > -----Message d'origine----- > De : Joseph Touch via Datatracker <nore...@ietf.org> > Envoyé : mercredi 8 mai 2024 06:06 > À : int-...@ietf.org > Cc : draft-ietf-opsawg-tsvwg-udp-ipfix....@ietf.org; last- > c...@ietf.org; opsawg@ietf.org > Objet : Intdir last call review of draft-ietf-opsawg-tsvwg-udp- > ipfix-08 > > > Reviewer: Joseph Touch > Review result: Ready with Issues > > Note that I previously performed an INTAREA early review on Jan > 12, 2024. > > This document sufficiently addresses the issues previously > raised, with the exception below. It introduces no new concerns. > > The sole remaining issue is the use of "unsigned256" as a data > type. This is necessary to represent the potential bitfield to > support all 256 possible UDP option Kind values. This document > cites draft-ietf-opsawg-ipfix-tcpo-v6eh as defining this type (in > Sec 8.3). That (tcpo-v6eh) document defines the range of this > type correctly, but then refers back to RFC7011 Sec 6.1.1 to > define the encoding. Unfortunately, RFC7011 defines encodings for > unsigned integers only up to 64 bits. > > This (udp-ipfix) document cites Section 6.2 of RFC7011 to define > reduced size encodings of unsigned256, but RFC7011 defines those > encodings only for 64, 32, and 16 bit unsigned integers. > Additionally, those reductions apply only when the high-order > byte(s) are all zero; for UDP options, the partitioning of > options into SAFE and UNSAFE groups and the assignment of > experiment codepoints to the highest values in both ranges > suggests that these reduced size encodings may be of limited > relevance. Med] Good point. As you can see in https://mailarchive.ietf.org/arch/msg/opsawg/JJAD7ooBNe_gFHRUAiHmK24HBNk/, we already made changes so that reduced-size encoding can be used even in the presence of shared options; see specifically this part: == [Med] These considerations are not specific to this document and fall under the generic considerations in base spec 7011 (Section 10.3.3). After think about this, I added a new section called (ops considerations) with the following content: * moved the text about reduced-size encoding to that section * encourage the use of reduced-size encoding in the presence of EXP/UEXP (that is the *ExID IEs) takes precedence and thus their flags can be ignored * add a pointer to transport (including MTU) IPFIX cons. == We can go one step further to export SAFE and UNSAFE options in separate IEs. > > At least one document needs to define unsigned256 normatively - > this doc, tcpo-v6eh, or some other. That document needs to > explain the byte order representation and reduced size encoding. [Med] Makes sense. Removed the pointer to 6.1.1 of 7011 and copy/paste the encoding in the tcp-eh I-D. > > This (udp-fix) document also uses the octetArray type, but then > defines it as being interpreted as "16-bit fields" (Sec 4.2, > 4.3). This should probably refer to unsigned16 values, but it's > not clear that this is a valid use of octetArrays. Being able to > read such an array as unsigned16 values may require half-word > (16bit) or word (32bit) alignment, such that reading an > unsigned16 from a misaligned offset may either result in an error > or a misinterpreted > (byte-swapped) value. Please provide an example of use of an > octetArray as a list of multi-octet values in a published RFC or > provide an alternate representation (or define one in detail). > [Med] Please see https://mailarchive.ietf.org/arch/msg/opsawg/XUeSAPc22OkAPKGJwO54BN2H5wM/. > Some additional minor suggestions: > > Note that the "ex" in ExID already means "experimental option". > It thus may be useful to consider shortening the name of the > related two element IDs, e.g., reducing > udpUnsafeExperimentalOptionExID to udpUnsafeExID and reducing > udpSafeExperimentalOptionExID to udpSafeExID. [Med] I prefer to keep the current names as we don't know if other IEs will be defined in the future to cover other aspects of these options. > > Fig 2 shows the full unsigned256 field, but it is not clear why > this is useful if the reduced length variant is sufficient. It > might be useful to show just the appropriate byte. [Med] The intent is to further insist on the reduced encoding. > > ____________________________________________________________________________________________________________ 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. _______________________________________________ OPSAWG mailing list -- opsawg@ietf.org To unsubscribe send an email to opsawg-le...@ietf.org
