PIM Null-Register Packing
draft-ietf-pim-null-register-packing-16

## COMMENT

Thanks for taking care of my DISCUSS, and also comments other than the one I've left below (and may amount to nothing, I just don't know).

I support Rob’s DISCUSS, and also his comments.

### General, error conditions

A related question to my DISCUSS is whether there are any error conditions that can apply to the non-packed versions of these messages, that need to be considered here. In a skim of RFC 7761 I didn't see anything problematic, but I didn't review it in detail. Anyway, my concern is *if* there are any error conditions that might apply, presumably they should apply only to the specific (S,G) and not to any other non-erroneous (S,G) encoded in the packed message. Sometimes error handling is written in terms of messages, is what got me thinking about it.

Thanks for clearing up the IANA stuff.

I support the DISCUSSes raised on the bit naming and confusion within the figures and on the Path MTU discovery.

Additionally, a comment:

     When a Register-Stop message with the P-bit set is received, the DR MAY send PIM Packed Null-Register messages

The following sentences then basically change this MAY to a RECOMMENDED, so a SHOULD would be more appropriate here.

    The RP, after receiving a PIM Packed Null-Register message, MAY start sending PIM Packed Register-Stop messages

Same here.

    In case the network manager disables the Packing Capability at the RP, or in other words, disables the feature from the RP, the router SHOULD NOT advertise the Packing Capability.

Why is this not a MUST NOT ?

I support the DISCUSS positions of John Scudder, Lars Eggert, Robert Wilton, and Zaheduzzaman Sarker to provide additional clarity in the specified behavior.

Thanks for clarifying P and FB.

# Éric Vyncke, INT AD, comments for draft-ietf-pim-null-register-packing-15
CC @evyncke

Thank you for the work put into this document. 

Please find below  some non-blocking COMMENT points (but replies would be appreciated even if only for my own education).

Special thanks to Mike McBride for the shepherd's detailed write-up including the WG consensus **and** the justification of the intended status. 

Other thanks to Donald Eastlake, the Internet directorate reviewer (at my request), please consider this int-dir "not ready" review:
https://datatracker.ietf.org/doc/review-ietf-pim-null-register-packing-14-intdir-telechat-eastlake-2023-02-28/ (and I have read Alvaro's and Ananya's replies and the fix in -15)

I hope that this review helps to improve the document,

Regards,

-éric


## COMMENTS

### IP versions

Should there be some text in sections 3 and 4 that the IP layer is not only used to get the amount of addresses but also the IP version of the addresses ?

### Section 7 normative ?

Is there a reason why section 7 does not use normative MAY and SHOULD ?

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

I support Robert's discuss on this.

# GEN AD review of draft-ietf-pim-null-register-packing-14

CC @larseggert

Thanks to Behcet Sarikaya for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/EyaOvVl155DHm7IhDZUBHOqRijY).

## Comments

### Section 2, paragraph 5
```
     Packing Capability bit (Flag Bit TBD1): When set, it indicates the
     ability of the RP to receive PIM Packed Null-Register messages, and
     send PIM Packed Register-Stop messages.
```
Is this the "P" bit in Figure 1? If yes, it seems like the position isn't TBD
anymore?

### Section 2, paragraph 4
```
  3.  PIM Packed Null-Register message format
        0                   1                   2                   3
         0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
        |PIM Ver| Type  |Subtype|  FB   |           Checksum            |
        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
```
What is "FB"? (Also in Figure 3.)

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Typos

#### "Abstract", paragraph 1
```
-    In PIM-SM networks PIM Null-Register messages are sent by the
+    In PIM-SM networks, PIM Null-Register messages are sent by the
+                      +
```

### Grammar/style

#### Section 4, paragraph 7
```
gister-Stop message sent to the DR. Thus a DR will switch to the new packet f
                                    ^^^^
```
A comma may be missing after the conjunctive/linking adverb "Thus".

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Updated.  Discuss cleared - thanks for addressing my discuss issue.

Regards,
Rob

----------

Previous comments:

Minor level comments:

(2) p 2, sec 2.  Packed Null-Register Packing Capability

   0                   1                   2                   3
   0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  |PIM Ver| Type  |P|6 5 4 3 2 1 0|           Checksum            |
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  |             Group Address (Encoded-Group format)              |
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  |            Source Address (Encoded-Unicast format)            |
  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      Figure 1: PIM Register-Stop message with Packing Capability option

I found the description of the flag bits to be unclear:
 - Are the "6 5 .. 1 0", the flag bits?
 - Is 'P' the "Packed Capability" bit?  If so, this diagram implies that it takes the value 7, but the text indicates that it hasn't been permanently assigned yet.


(3) p 3, sec 3.  PIM Packed Null-Register message format


      0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |PIM Ver| Type  |Subtype|  FB   |           Checksum            |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |     Group Address[1]   (Encoded-Group format)                 |
      |     Source Address[1]  (Encoded-Unicast format)               |
      .                                                               .
      .                                                               .
      .                                                               .
      .                                                               .
      .     Group Address[N]                                          .
      |     Source Address[N]                                         |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
              Figure 2: PIM Packed Null-Register message format

Unclear what FB means, "Flag Bits"?  Also in section 4.



Nit level comments:

(4) p 4, sec 5.  Protocol operation

                                                             However, it
      is RECOMMENDED to stick to the packed format as long as the RP and
      DR have the feature enabled.

As an editorial nit, I would suggest combining these two sentences together to make it clear what "the decision" is referring to.  The same comment also applies to the pragraph below.

Regards,
Rob