Re: [MMUSIC] AD Review: draft-ietf-mmusic-ice-sip-sdp

[Suhas Nandakumar <[e-mail]>]

Thanks Adam for the review. I have submitted version-36 to address the
feedback.

Cheers
Suhas

On Thu, Jun 6, 2019 at 6:11 PM Adam Roach <[e-mail]> wrote:

> This is my AD review for draft-ietf-mmusic-ice-sip-sdp.
>
> Thanks to everyone for the huge amount of work invested in updating
> these ICE procedures. My review did not turn up any technical issues, and
> I will be progressing the document to IETF Last Call momentarily.
>
> I have a fair number of minor changes that should be made to the
> document.  Treat them as you would any other IETF last call comments.
>
> ---------------------------------------------------------------------------
>
> §2:
>
>  >  The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
>  >  "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
>  >  "OPTIONAL" in this document are to be interpreted as described in RFC
>  >  2119 [RFC2119].
>
> Please update this paragraph to use the text from RFC 8174.
>
> ---------------------------------------------------------------------------
>
> §3.1:
>
>  >  corresponds to the [RFC3264] Offer/Answer protocol and the
>  >  terminologies offerer and answerer correspond to the initiator and
>  >  responder terminologies from [RFC8445] respectively.
>
> Nit: this is a somewhat unconventional use of the word "terminologies,"
> and is
> slightly awkward in a couple of other ways as well.  Suggest updating as:
>
>     corresponds to the [RFC3264] Offer/Answer protocol, and the
>     terms "offerer" and "answerer" correspond to the initiator and
>     responder roles from [RFC8445] respectively.
>
>
>  >  Once the initiating agent has gathered, pruned and prioritized its
>
> Nit: "...gathered, pruned, and prioritized..."
>                           ^
>
> ---------------------------------------------------------------------------
>
> §3.2.1.2:
>
>  >  Within a "m=" section, each candidate (including the default
>
> Nit: ...an "m=" section...
>           ^
> ---------------------------------------------------------------------------
>
> §3.2.1.6:
>
>  >  bandwidth value of zero [RFC4566], the agent MUST still include ICE
>  >  related SDP attributes.
>
> Nit: "...ICE-related..."
>
> ---------------------------------------------------------------------------
>
> §3.3.2:
>
>  >  the answerer MUST NOT include any ICE related SDP attributes in the
>
> Nit: "...ICE-related..."
>
>  >  If the answerer detects a possibility of the ICE mismatch, procedures
>
> Nit: "...possibility of an ICE mismatch..."
>                          ^^
>
>  >  Note:  <draft-holmberg-ice-pac>> provides guidance on finding working
>
> Nit:  ...[draft-holmberg-ice-pac]...
>
> ---------------------------------------------------------------------------
>
> §3.3.3:
>
>  >  On the other hand, if the answer indicates the support for ICE but
>
> Nit: "...indicates support for ice..."
>
>  >  in the answer, as described in (Section 3.2.5), the offerer MUST
>
> Nit: "...described in Section 3.2.5, the offerer MUST..."
>
>  >  Note:  <draft-holmberg-ice-pac>> provides guidance on finding working
>
> Nit:  ...[draft-holmberg-ice-pac]...
>
> ---------------------------------------------------------------------------
>
> §3.4.1.1.1:
>
>  >  An agent sets the rest of the ice related fields in the SDP for this
>
> Nit: "...ICE-related..."
>
> ---------------------------------------------------------------------------
>
> §3.4.1.1.3:
>
>  >  the SDP for this data stream as if this was an initial offer for that
>
> Nit: "...as if this were..."
>
> ---------------------------------------------------------------------------
>
> §3.4.1.2.1:
>
>  >  gathered since the last offer/ answer exchange, including peer
>
> Nit: "...offer/answer..."
>
> ---------------------------------------------------------------------------
>
> §3.4.1.2.1:
>
>  >  connection address, port and transport protocol in each "c=" and "m="
>
> Nit: "...connection address, port, and transport protocol..."
>                                   ^
> ---------------------------------------------------------------------------
>
> §3.4.1.3:
>
>  >  If the ICE state is running, a lite implementation MUST include all
>
> Nit: ...state is "running", a lite...
>
>  >  formed identical to the procedures for initial offers.
>
> Nit: "...identically..."
>
>  >  it MUST restart ICE.  Similarly, the username fragments or passwords
>
> Nit: "...fragments and passwords..."
>
> ---------------------------------------------------------------------------
>
> §3.4.3.1:
>
>  >  media server during call hold using 3rd party call-control
>  >  procedures.  Omitting further details how this is done, this could
>
> Nit: cite RFC 3725 here.
>
> ---------------------------------------------------------------------------
>
> §3.4.3.1:
>
>  >     If a pair on the new check list was
>  >     also on the previous check list, and its state is not Frozen, its
>  >     state is copied over.  Otherwise, its state is set to Frozen.
>
> How is this procedure different than the simpler (and equivalent sounding):
>
>       If a pair on the new check list was also on the previous check list,
>       its state is copied over.  Otherwise, its state is set to Frozen.
>
>
> ---------------------------------------------------------------------------
>
> §3.4.3.1.1:
>
>  >  The agent MUST remember the nominated pair in the Valid list for each
>  >  component of the data stream, called the previous selected pair prior
>  >  to the restart.
>
> This is hard to parse. I think it means to say:
>
>     The agent MUST remember the nominated pair in the Valid list for each
>     component of the data stream, called the "previous selected pair",
> prior
>     to the restart.
>
> (note the addition of quotation marks and a comma)
>
> There's a similar confusing lack of quotation marks in section 3.4.3.2.
>
> ---------------------------------------------------------------------------
>
> §4.1:
>
>  >  <component-id>:  is a positive integer between 1 and 256 (inclusive)
>  >     that identifies the specific component of the dta stream for which
>
> Nit: "...data stream..."
>
>
>  >  a reference to the document defining the usage of the extension
>
> Nit: missing ending period.
>
> ---------------------------------------------------------------------------
>
> §4.4:
>
>  >  The ice-ufrag attribute MUST contain at least 24 bits of randomness,
>  >  and the ice-pwd attribute MUST contain at least 128 bits of
>  >  randomness.
>
> This is reiterating normative requirements from RFC 8445 section 5.3.
> In general, reiterating normative behavior from a related specification
> should be avoided.  I would suggest rephrasing as:
>
>     RFC 8445 requires the ice-ufrag attribute to contain at least 24 bits
> of
>     randomness, and the ice-pwd attribute to contain at least 128 bits of
>     randomness.
>
> ---------------------------------------------------------------------------
>
> §4.6:
>
>  >  Example shows 'rtp+ecn' ice-option SDP line from <<RFC6679>>:
>
> Nit: [RFC6679]
>
> Also, add RFC 6679 to the "Informative References" section.
>
> ---------------------------------------------------------------------------
>
> §6.1.1:
>
>  >  the session terminated.  For the ICE lite peers , the agent MUST
>
> Nit: remove the space before the comma.
>
> ---------------------------------------------------------------------------
>
> §8.2.1:
>
>  >  Require Header Field in their offer.  UAs that rejects non-ICE offers
>
> Nit: "...that reject non-ICE..."
>
>  >  SHOULD use a 421 response code, together with an Option Tag "ice" in
>  >  the Require Header Field in the response.
>
> This normatively reiterates normative language in RFC 5768. Please
> change the "SHOULD" to a "will generally" or something similar.
>
> ---------------------------------------------------------------------------
>
> §11.1:
>
>  >  [RFC8126]  Cotton, M., Leiba, B., and T. Narten, "Guidelines for
>  >             Writing an IANA Considerations Section in RFCs", BCP 26,
>  >             RFC 8126, DOI 10.17487/RFC8126, June 2017,
>  >             <http://www.rfc-editor.org/info/rfc5226>.
>                                                   ^^^^
>
> Nit: the URL should point to RFC 8126.
>
> ---------------------------------------------------------------------------
>
> §11.3:
>
> This section provides no new information, and should be removed.
>
> ---------------------------------------------------------------------------
>
> Appendix D:
>
>  >  This begs the question -- why is the updated offer/answer exchange
>
> Nit: "This raises the question..."
>
> _______________________________________________
> mmusic mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/mmusic
>