Subject: [bfcpbis] PROTO review comments on draft-ietf-bfcpbis-rfc4583bis-08

From: Mary Barnes <[email-masked]>
Date: Wed, 18 Dec 2013 16:38:03 -0600

Hi all,

I have agreed to shepherd this document on behalf of the WG.  I have
reviewed this document in preparation for doing the PROTO write-up.

I think the document needs a bit of work before it's ready to progress.  I
have a few questions/comments, as well as editorial nits which I think
would improve readability and ease of understanding.

*General Comments:*

The document requires an SDP directorate review prior to progressing.  I
have requested that of the MMUSIC WG chairs.   At the time of this review,
Ali Begen has agreed to do that review.

*Comments/Questions:*

1) I am slightly puzzled by the following in section 3:

m=<media> <port> <transport> <fmt> ...

Section  5.14 of RFC 4566 states the following:

m=<media> <port> <proto> <fmt> ...

So, this looks to be like the RFC 2327 ABNF for m lines is being used
(rather than RFC 4566 which is the normative SDP specification for this
document)?

m=<media> <port> <transport> <fmt list>

Note, that the IANA registration section appears to be based on RFC 4566 as
the values are specified as being applicable to the 'pro to' field.  So,
 my guess is that this is a bug from RFC 4583.

2) Section 3, next to last paragraph. I think it would be more precise to
reword this as:

OLD:

    The fmt (format) list is ignored for BFCP.  The fmt list of BFCP 'm'
lines SHOULD contain a single "*" character.

NEW:

   The fmt (format) list is not applicable to BFCP.   The fmt list of 'm'
lines in the case of any proto field value related to BFCP SHOULD contain a
single "*" character.  If the the fmt list contains any other value it is
ignored.

3)  Section 4, paragraph above table 1, 1st sentence.  I suggest for
clarification to make the following change:

OLD

    MUST include one in the corresponding media description

NEW

    MUST include a 'floorctrl' attribute in the corresponding media
description

4) Section 4, 3rd paragraph from the end:

    "Endpoints that use the offer/answer model to establish BFCP
connections MUST support the 'floorctrl' attribute.  A floor control server
acting as an offerer or as an answerer SHOULD include the attribute in its
session descriptions."

Since the latter is a SHOULD what happens if the attribute is NOT included?
Under what circumstances is it okay for the server not to include it?    IF
bad things happen if it's not included, then this probably ought to be a
MUST.   Or, if there's reasonable situations in which the floor control
server doesn't include the attribute, those should be explained or at least
an example provided.

5)  Section  7.

a)  1st paragraph after ABNF.  "..versions SHOULD be integers..".   I would
think that ought to be a MUST.  What happens if the token isn't an integer
and what was the motivation for not defining the version as an integer
versus a token?   If non-integers can be used, the scope of what is
acceptable for the field needs to be explained.

b) 2nd paragraph after ABNF.  I can see why supporting the version is a
SHOULD (since it is a new field) and RFC 4583 did not define a version, but
I think that needs to be more clearly documented and I think it ought to be
REQUIRED for anyone that is using UDP and obviously, older implementations
of TCP (i.e., those only compliant to RFC 4583 and not this document) ought
to be the only ones that aren't using the version field.

c) last paragraph.  Related to b, I would think that in the case of
unreliable transports, it's not that an endpoint MUST "assume".  An
endpoint MUST "use" and signal a version number of 2.   In the case of a
reliable transport, if the endpoint does not signal a
bfcpversion-attribute, then the floor control server MUST use a value of 1.
  I'm recalling a fair amount of mailing list discussion on this one, so if
you can point to the thread where this text was agreed, I can let this one
go.  I still think it's not specified quite right, but I can live with it.

6) Section 8.  1st sentence.  I think this should be a "can use TCP or UDP"
and not a "may use TCP or UDP". Note, there is the perennial debate with
regards to whether something is normative even when it's not CAPS.  I
prefer to just use a different word to remove any confusion, particularly
when a different word is actually more correct.

7) Section 8.1,  4th paragraph, 1st sentence and last sentence.  I think
the two "SHOULD generate and offer" ought to be a MUST generate an offer OR
you need to specify the circumstances under which a client would not
generate an offer.

8) Section 9, 1st paragraph.  You're going to have to be more specific
about the potential mechanisms for authentication - at least provide an
example of mechanisms - i.e., I don't think "some mechanism" will fly with
the SecDir folks.

9) Section 11, 2nd paragraph.  I think that the assumes in the following
statement needs to be a required.  I suggest the following change:

OLD

  BFCP assumes that an initial integrity-protected channel is used to
exchange...

NEW

 An initial integrity-protected channel is REQUIRED for BFCP to exchange...

10) Related to item 7, this list of changes has no mention of the new
"bfcpver" attribute.  That definitely should be on this list.

*Editorial nits:*

1) Introduction.  "These data includes..." -> "This data includes...."
 (data is now grammatically considered to be a singular noun).

2) Section 3.  1st paragraph after the indented text.  This could be
written more concisely (and per technical doc standards not as the 1st
person) as follows (and per comment 1) above, I think this should be the
'proto' field:

OLD:

     We define four new values for the transport field:

NEW:

      This document defines four values for the proto field:

3) Section 5.  I suggest the following change:

OLD:

     We define the 'confid' and the 'userid' SDP media-level attributes.

NEW:

   This document defines two SDP media-level attributes: 'confid' and
'userid'.

4) Section 6. I suggest the following change:

OLD:

    We define the 'floorid' SDP media-level attribute.

NEW:

   This document defines the 'floorid' SDP media-level attribute.

5) Section 13.  I found this really, really hard to read.  I suggest you
use a bulleted or numbered list versus this hanging list style.