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

[Tom Kristensen <[e-mail]>]

All the issues here are resolved inline in line with Mary's suggestions -
as adjusted and commented by Charles and myself in this thread. The
upcoming version will include the changes.

-- Tom


On 24 January 2014 23:41, Charles Eckel (eckelcu) <[e-mail]> wrote:

> On 1/16/14 6:47 AM, "Tom Kristensen (tomkrist)" <[e-mail]>
> wrote:
>
> >I went through and commented on this in my favourite editor (Emacs
> >obviously), but forgot to paste it in and respond until now. I have
> >updated my comments to align with Charles Eckel's  recent response too.
>
> Thanks for that. More inline.
>
> >
> >Inline below.
> >
> >
> >On 12/18/2013 11:38 PM, Mary Barnes wrote:
> > > 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.
> >
> >Yes, I have commented on his issues on the MMUSIC mailing list.
> >
> > > _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.
> >
> >Yes, an inherited bug that will be fixed without complications.
> >
> > > 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.
> >
> >Agree. That is a more precise wording for the fmt part.
> >
> >Charles Eckel touched upon this as well and was okay with this,
> >especially if there are known reasons and/or implementations that
> >already do otherwise. Well, once upon a time there was a SIP based
> >product from a (large french) vendor that used 0 (I think) - don't think
> >that product is still maintained. But it might be out there in the wild
> >still. Will fix.
> >
> > > 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
> >
> >Fine with me, will update.
> >
> > > 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.
> >
> >The default behaviour/assumption if the 'floorctrl' attribute is not
> >used is explained in the paragraph below, i.e. offerer == client and
> >answerer == server.
> >
> >I agree that the formulation "is not used in an offer/answer exchange"
> >might point towards usage other than offer/answer, but the sentence
> >continues with explaining what the offerer and answerer will assume. So
> >fine with me. OK?
> >
> >If not, I will not object using MUST here - as also proposed by Charles
> >Eckel.
> >
> > > 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.
> >
> >I agree, this will be upgraded to a MUST if people doesn't object.
> >
> >And if this will ever change non-integer versions must be specified in
> >another draft/RFC and this draft probably have to be updated as well -
> >since semantics for specific versions are specified later in Section 7.
> >
> > > 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.
> >
> >Is it sufficient to merge in the following sentence after the second
> >sentence in that paragraph?
> >"However, endpoints that supports RFCXXXX, and not only the RFC 4583
> >subset, is REQUIRED to support and use the 'bfcpver' attribute."
> >(Or something along those lines).
>
> Works for me, though should be "endpoints Š are".
>
> >
> >A complicating factor here is that some pre-standard implementations, as
> >experienced on recent SIPit and SuperOp test events, is out there and is
> >not using the 'bfcpver' attribute. However, demanding that
> >implementations following the new RFC from this draft have to use the
> >version attribute should be perfectly fine.
>
> Agreed, and stating that when omitted the default is "1" for TCP and "2"
> for UDP addresses the problem of pre-standard UDP implementations as well.
>
> >
> > > 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.
> >
> >I can't find too much discussion about this, but I agree that the word
> >"assume" is not good here. What about reformulating this last paragraph
> >to:
> >
> >"If a 'bfcpver' attribute is not present, default values are based on
> >the transport specified in the m-line (Section 3). When used over an
> >unreliable transport, an endpoint MUST use a value of "2", and when used
> >over a reliable transport, an endpoint MUST use a value of "1". This is
> >in line with the definition of the Version field in [8]."
> >
> >Any better?
>
> Better, but how about:
>
> "If a 'bfcpver' attribute is not present, default values are inferred from
> the transport specified in the m-line (Section 3). In accordance with
> definition of the Version field in [8], when used over a reliable
> transport the default value is "1", and when used over an unreliable
> transport the default value is "2".
>
>
> >
> > > 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.
> >
> >Well explained, the change is fine with me.
> >
> > > 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.
> >
> >OK
> >
> > > 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.
> >
> >Too bad, since this is inherited from RFC 4583 - but of course that text
> >wasn't perfect ;)
> >Anyway, isn't the mechanisms meant here mentioned in the paragraphs
> >below? If so, might extending to "using some mechanism, as explained
> >below" be a valid solution in front of the SecDir review?
> >
> >Or as Charles Eckel proposed, pointing towards TLS/DTLS as the preferred
> >mechanism, but other mechanisms exists but are outside the scope of this
> >document.
>
> This still works for me :)
>
> Cheers,
> Charles
>
> >
> > > 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...
> >
> >Yes, I'll adopt that one. No assumption, we will require!
> >
> > > 10) Related to item 7, this list of changes has no mention of the new
> > > "bfcpver" attribute. That definitely should be on this list.
> >
> >Indeed! It's missing since it arrived late, but that's just a poor
> >excuse...
> >
> >
> > > _Editorial nits:_
> > >
> > > 1) Introduction. "These data includes..." -> "This data includes...."
> > > (data is now grammatically considered to be a singular noun).
> >
> >OK
> >
> > > 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:
> >
> >OK
> >
> > > 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'.
> >
> >OK
> >
> > > 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.
> >
> >OK
> >
> > > 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.
> >
> >Sure, I'll tweak the XML curses and make it more readable.
> >
> >
> >-- Tom
> >_______________________________________________
> >bfcpbis mailing list
> >[e-mail]
> >https://www.ietf.org/mailman/listinfo/bfcpbis
>
> _______________________________________________
> bfcpbis mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/bfcpbis
>



-- 
# Cisco                         |  http://www.cisco.com/telepresence/
## [e-mail]  |  http://www.tandberg.com
###                               |  http://folk.uio.no/tomkri/