Subject: Re: [sipcore] WGLC: draft-ietf-sipcore-location-conveyance

From: "Stach, Thomas" <[email-masked]>
Date: Mon, 7 Mar 2011 17:15:16 +0100

Hi James, Brian, Jon, 

I did some review with focus on section 4. Please find below some comments:

General/Editorial:

The document uses the term "header" and "header field" interchangeably. 
I propose to consistently use "header field".
See e.g. captions of section 4.1, 4.2, 4.4, 8.1, 8.2. 8.5 and several occurrences in the text.

Section2, 
1st para
"... of a Target (Target) to a Location Recipient (LR)."
(Target) does not seem to abbreviate something.

2nd para
"In order to convey location information, this document specifies a new SIP header, the Geolocation header, which ..." 
Since the document defines 3 header fields, what about:
"In order to convey location information, this document specifies three
new SIP header fields, Geolocation, Geolocation-Routing and Geolocation-Error, which carry a reference to
a Location Object, grant permission to route a SIP request based on the loation-value and provide error 
notifications specific to location errors. "
Note, this text proposal still does not mention the new 424 Response code, which could be mentioned too.

ABNF issues in section 4

section 4.1
message-header    /= Geolocation-header ; (message-header from 3261)
Geolocation-header = ...

Related to my comment above I propose to rename Geolocation-header to Geolocation.  
This would also be more inline with the 3261 practice of header field definitions.

locationURI        =  sip-URI / sips-URI / pres-URI
                          / http-URI / HTTPS-URI

What about changing HTTPS-URI to https-URI in order to make consistent use of capitalization or is this too much nitpicking?

"   HTTP-URI and HTTPS-URI are defined according to [RFC2616] and
   [RFC2818], respectively."

a. There is inconsistent capitalization of "HTTP-URI" when compared to the ABNF (another nit).  
b. While 2616 provides ABNF for http-URI, 2818 does not provide ABNF for https-URI.
   Would it makes sense to provide a corresponding ABNF production in this doc?
   e.g.  https_URI = "https:" "//" host [ ":" port ] [ abs_path [ "?" query ]]

"If a
   Geolocation header field is received that contains generic-params,
   each SHOULD be ignored, and SHOULD NOT be removed when forwarding
   ^^^^
   the locationValue."

Does "each" refer to the header field or parameter? 
Though rather obvious that the parameter is meant changing 
"... each SHOULD be ignored..."  to 
"... each parameter SHOULD be ignored..." would make that clearer.

section 4.2

message-header    /= Georouting-header ; (message-header from 3261)
Georouting  = "Geolocation-Routing" HCOLON
                        ( "yes" / "no" / generic-value )

a) Propose to rename Georouting-header to Georouting (or Geolocation-Routing), as per above.
b) I could not find a definition/production/reference for generic-value.
Do you mean gen-value from 3261? I.e gen-value = token / host / quoted-string?
If yes, do you really need this flexibility or would e.g. quoted-string alone be sufficient?

The para after the ABNF definition end with:
" ... Values other than "yes" or "no" are permitted as a
   mechanism for future extensions, and should be treated the same as
   "no". "  
Please add "if not understood" after "no".

Section 4.4

   message-header          /= Geolocation-Error-header ;
                                (message-header from 3261)
   Geolocation-Error        = "Geolocation-Error" HCOLON
                                locationErrorValue
   locationErrorValue       = location-error-code
                               *(SEMI location-error-params)
   location-error-code      = 1*3DIGIT
   location-error-params    = location-error-code-text
                              / generic-param ; from RFC3261
   location-error-code-text = "code" EQUAL quoted-string ; from RFC3261

Geolocation-Error-header is not further expanded. You probably mean Geolocation-Error.

The examples do not conform to this ABNF, e.g. 
Geolocation-Error: 100 "Cannot Process Location" 
does not show the semicolon, the string "code" and the "="
According to the ABNF is would have to read 
Geolocation-Error: 100 ; code="Cannot Process Location" 
                      ^^^^^^^^
- A further question on 
locationErrorValue       = location-error-code *(SEMI location-error-params) 
Does the locationErrorValue really need one or more parameters or would a simple reason phrase be sufficient? 
The current example suggest the latter, whereas the ABNF suggests the former. 
If it is just a reason phrase one could simplify to:
locationErrorValue       = location-error-code [SP location-error-code-text]
location-error-code-text = quoted-string 
However I don't have a strong preference here, other than that the examples should match the ABNF.

- The grammar of RFC 3261 specified explicit ABNF for the response codes. 
Do you want to do the same for location-error-code, which currently only specifies 1*3DIGIT?
The effort of doing so does not seem to be too big for the five already defined codes 100, 200, 300, 201, 202.

- From these specified error codes and their explaining text on page 15/16 it does not become clear 
if the text phrase is a only default string or a fixed value. 
Similar to RFC 3261 I assume the former is meant and not the latter. 
However, if it was the latter this should be nailed down in the ABNF.

Kind Regards

Thomas

> -----Ursprüngliche Nachricht-----
> Von: [email-masked] 
> [mailto:[email-masked]] Im Auftrag von Adam Roach - 
> SIPCORE Chair
> Gesendet: Dienstag, 01. März 2011 00:38
> An: SIPCORE (Session Initiation Protocol Core) WG
> Cc: [email-masked]; 
> Robert Sparks; SIPCORE Chairs
> Betreff: [sipcore] WGLC: draft-ietf-sipcore-location-conveyance
> 
> [as chair]
> 
> The current editor of draft-ietf-sipcore-location-conveyance believes 
> that the document has no remaining technical issues[1], and 
> is ready for 
> evaluation. Today, we are starting a two-week working group last call 
> period. This last call period ends on Tuesday, March 15th.
> 
> The latest version of the document can be retrieved here:
> 
> http://tools.ietf.org/html/draft-ietf-sipcore-location-conveyance
> 
> Any comments on the document should be sent to the SIPCORE 
> mailing list.
> 
> /a
> 
> [1] John Elwell's editorial comments of February 25th have 
> been noted by 
> the authors, and will be treated as WGLC comments.
> _______________________________________________
> sipcore mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/sipcore
>

---

From: Robert Sparks <[email-masked]>
Date: Wed, 9 Mar 2011 10:24:12 -0600

James -

There are some nits that idnits points out - please address those with the other WGLC comments.

RjS

On Feb 28, 2011, at 5:38 PM, Adam Roach - SIPCORE Chair wrote:

> [as chair]
> 
> The current editor of draft-ietf-sipcore-location-conveyance believes that the document has no remaining technical issues[1], and is ready for evaluation. Today, we are starting a two-week working group last call period. This last call period ends on Tuesday, March 15th.
> 
> Any comments on the document should be sent to the SIPCORE mailing list.
> 
> /a
> 
> [1] John Elwell's editorial comments of February 25th have been noted by the authors, and will be treated as WGLC comments.

---

From: Robert Sparks <[email-masked]>
Date: Wed, 9 Mar 2011 14:45:27 -0600

There are a couple of larger and several small things to also address in this document before requesting publication.

The larger things:

1) In section 4.4, the document talks about providing a text string "RECOMMENDED for human readability". The document
needs to be clear who this is intended for. Do you expect this to be rendered to a user or only read by an operator?
(Fortunately quoted-string allows non-ascii UTF-8 already, so we won't have to argue about whether this needs to be
internationalized.)

2) The Geolocation-Error ABNF and the IANA registration section use a parameter name = value form (with a name of "code").
Following those sections of the document, the examples should all look like
Geolocation-Error: 100;code="Cannot Process Location"
But all the examples currently look like
Geolocation-Error: 100 "Cannot Process Location"

It's worth pointing out that 3261 (see the top of page 32) doesn't allow the same parameter name to appear in
a header field value more than once, so the following is not allowed:
Geolocation-Error: 100;code="Cannot Process Location";code="Impossible de processus de localisation"

Are the values you are registering for code= suggested defaults (like for SIP response start lines) or are
they what MUST appear? Is this OK?
Geolocation-Error: 100;code="Have a nice day"
If not, what in the document says so?

3) I can't figure out what the paragraph in section 4.4 that starts "Additionally, if a sip entity cannot..." and goes
onto talk about 503's is trying to say. I think some surrounding context must have been lost or not captured.
Why would we be recommending sending a 503 here?

4) Section 4.4 defines classes of error codes, and even default values of error codes, but does not go on to say
that if an element receives an error code it doesn't understand, it treats it like the default code in that class.
Surely that was the intent (otherwise, having the classes isn't particularly useful). If it was, the document should
say so.

---------------------------------------------------------

Here are the smaller things.
I've listed these in as close to document order as I could

1) In 4.1, please call out what document's definition of HCOLON, COMMA, LAQUOT, RAQUOT and SEMI that you are using.
    Either call out that these are used in other places in the doc (4.2 and 4.4) or similarly call out the terminals used in those
    sections (be sure to catch DIGIT and EQUAL in 4.4).

2) In section 4.1, I suggest replacing
OLD: A Geolocation header field MUST have at least one header-value.
with
NEW: A Geolocation header field MUST have at least on

3) In 4.2, I suggest replacing
OLD: Values other than "yes" or "no" are permitted as a mechanism for future extensions, and should be treated the same as "no".
with
NEW: The syntax allows values other than "yes" or "no" to appear to allow for future extensions. Implementations not aware of an extension SHOULD treat any other received value the same as "no".

I also propose that this be MUST instead of SHOULD.

4) 4.2.1, first paragraph, s/contained in a one or/contained in one or/ (delete the spurious "a")

5)  I suggest the following replacement for the paragraph in section 4.3 that starts "It is only appropriate"

It is only appropriate to generate a 424 response when the responding entity needs a locationValue
and there are no values in the request that are usable by the responder, or when the responder has
additional location information to provide. The latter case is shown in Figure 4 of section 3.4. There,
a SIP intermediary is informing the upstream UA which location to include in the next SIP request.

6) In 4.4 was "any SIP non-100 response" intended to mean
   a) any SIP response, including provisional responses other than 100 Trying
   or
   b) only SIP Final responses
   ? 

7) Section 8.1 claims two actions for IANA, but provides only one.

8) In section 8.5 step 2, please just say "Yes" under predefined values instead of "yes*", and
delete the footnote. Section 8.6 is the next section and is easy to find, and it will save
IANA having to ask if you are trying to put a * in the table in the registry.

9) A.2 UAS-1 will cause confusion. We are allowing locations to appear in responses, just not
    the location of the responder. Please adjust it to avoid that potential confusion.
    
    As an individual contributor, I don't think these appendices add enough value to the document 
    to warrant the confusion they may cause and would be happier if they were deleted.

---

From: Robert Sparks <[email-masked]>
Date: Wed, 9 Mar 2011 15:46:53 -0600

Hi document editors -

Here are a few additional suggestions that are purely editorial that I think would 
make the document stronger - please consider them, but its up to you whether to 
do anything with them.

a) I suggest "your favorite local pizza delivery service" instead of "Pizza Hut"

b) Replace 'The only conceivable way forward, when a second location is placed into
the same SIP request by a SIP intermediary is to take a "you break it, you bought it" philosophy
with respect to the inserting SIP intermediary' with 'This document takes a "you break it, you bought it"
approach to dealing with second locations placed into a SIP request by an intermediary entity.'

c) Delete the parenthetical '(we are not going to discuss any other reasons in this document, and
there are many)'. That's obvious and distracts from the point.

d) Replace 'SIP intermediaries are NOT RECOMMENDED to modify existing locationValue(s),
and further not to delete any either' with 'SIP intermediaries SHOULD NOT modify or remove any
existing locationValue(s).'

e) Replace the first sentence of the paragraph at the end of page 10 with
'A Geolocation-Routing header-value that is set to "no" has no special security properties. It is
at most a request for behavior within SIP intermediaries.'

f) Replace 'SIP intermediaries MUST NOT add, modify or delete the location in a 424 response.'
with 'SIP intermediaries that are forwarding (as opposed to generating) a 424 response MUST
NOT add, modify, or delete any location appearing in that response.'

g) Delete the '- etc...' bullet in the non-exclusive list of reasons for 1XX in section 4.4
 
h) Delete 'At this time,' in the paragraph after that list. This document won''t alter things at
any other time either. 

i) Replace "This was first brought up in section 3.2." with "The end of section 3.2 discusses
how transaction timing considerations lead to this requirement."

RjS

---

From: "James M. Polk" <[email-masked]>
Date: Wed, 09 Mar 2011 17:54:04 -0600

Robert

sure

The only one I know I cannot address is the lack of form feeds per 
page. My dumb text editor  doesn't allow me to insert them.

James

At 10:24 AM 3/9/2011, Robert Sparks wrote:
> > The current editor of draft-ietf-sipcore-location-conveyance 
> believes that the document has no remaining technical issues[1], 
> and is ready for evaluation. Today, we are starting a two-week 
> working group last call period. This last call period ends on 
> Tuesday, March 15th.
> >
> > [1] John Elwell's editorial comments of February 25th have been 
> noted by the authors, and will be treated as WGLC comments.

---

From: "James M. Polk" <[email-masked]>
Date: Wed, 09 Mar 2011 18:20:37 -0600

Robert

Thanks for the scrubbing.

These will be part of the post WGLC version.

James

At 02:45 PM 3/9/2011, Robert Sparks wrote:
>There are a couple of larger and several small things to also 
>address in this document before requesting publication.
>
>The larger things:
>
>1) In section 4.4, the document talks about providing a text string 
>"RECOMMENDED for human readability". The document
>needs to be clear who this is intended for. Do you expect this to be 
>rendered to a user or only read by an operator?
>(Fortunately quoted-string allows non-ascii UTF-8 already, so we 
>won't have to argue about whether this needs to be
>internationalized.)
>
>2) The Geolocation-Error ABNF and the IANA registration section use 
>a parameter name = value form (with a name of "code").
>It's worth pointing out that 3261 (see the top of page 32) doesn't 
>allow the same parameter name to appear in
>Geolocation-Error: 100;code="Cannot Process 
>Location";code="Impossible de processus de localisation"
>
>Are the values you are registering for code= suggested defaults 
>(like for SIP response start lines) or are
>3) I can't figure out what the paragraph in section 4.4 that starts 
>"Additionally, if a sip entity cannot..." and goes
>onto talk about 503's is trying to say. I think some surrounding 
>context must have been lost or not captured.
>4) Section 4.4 defines classes of error codes, and even default 
>values of error codes, but does not go on to say
>that if an element receives an error code it doesn't understand, it 
>treats it like the default code in that class.
>Surely that was the intent (otherwise, having the classes isn't 
>particularly useful). If it was, the document should
>say so.
>
>1) In 4.1, please call out what document's definition of HCOLON, 
>COMMA, LAQUOT, RAQUOT and SEMI that you are using.
>     Either call out that these are used in other places in the doc 
> (4.2 and 4.4) or similarly call out the terminals used in those
>OLD: Values other than "yes" or "no" are permitted as a mechanism 
>for future extensions, and should be treated the same as "no".
>with
>NEW: The syntax allows values other than "yes" or "no" to appear to 
>allow for future extensions. Implementations not aware of an 
>extension SHOULD treat any other received value the same as "no".
>

---

From: "James M. Polk" <[email-masked]>
Date: Wed, 09 Mar 2011 18:21:09 -0600

Robert

Thanks for the additional scrubbing.

These will also be part of the post WGLC version.

James

---

From: Paul Kyzivat <[email-masked]>
Date: Wed, 09 Mar 2011 19:26:25 -0500

Robert,

Thanks for the very thorough review.
I agreed with everything you brought up.

	Thanks,
	Paul

---

From: Robert Sparks <[email-masked]>
Date: Wed, 9 Mar 2011 21:48:46 -0600

Hi James -

I spotted one thing below that got mangled before I sent it - here's the correction:

>> There are a couple of larger and several small things to also address in this document before requesting publication.
<snip/>
>> I've listed these in as close to document order as I could
<snip/>
>> NEW: A Geolocation header field MUST have at least on

That got truncated somehow before I sent it. It should have said:

NEW: A Geolocation header field MUST have at least one locationValue.

RjS

---

From: "James M. Polk" <[email-masked]>
Date: Thu, 10 Mar 2011 00:09:40 -0600

At 09:48 PM 3/9/2011, Robert Sparks wrote:
>Hi James -
>
>I spotted one thing below that got mangled before I sent it

I saw that when I glanced initially. I figured I'd ask about it once 
I dove into the edit. Thanks for correcting before I got into it.

James

>- here's the correction:

---

From: "James M. Polk" <[email-masked]>
Date: Tue, 29 Mar 2011 03:47:05 -0500

>Why would we be recommending sending a 503 here?

pick on this one point, there was an error code for that basically 
said "couldn't process the location you just sent me, but it's ok to 
try again" *and* "couldn't process the location you just sent me, 
don't try to resend it for a while". Jon didn't like the idea of 
something that he feels duplicates the 503 with a retry after header 
field. He was on a mission to reduce the number of geolocation-error 
codes, and these two go whacked. That's where this text comes from. 
I'll get with him about writing this section better - or do you have 
an alternate plan we should pursue?

James

---

From: Robert Sparks <[email-masked]>
Date: Tue, 29 Mar 2011 13:59:32 +0200

Thanks for the reminder - I reread the text and talked with Jon. I think the only thing that needs to change
is the actual error code. 503 is to extreme - it would cut off all SIP traffic from the peer receiving the response,
not just traffic related to this request. I think the right code in this context is 500 (which can also use the Retry-After
mechanism).

More concisely, my suggestion is s/503/500/

RjS

On Mar 29, 2011, at 10:47 AM, James M. Polk wrote:

> pick on this one point, there was an error code for that basically said "couldn't process the location you just sent me, but it's ok to try again" *and* "couldn't process the location you just sent me, don't try to resend it for a while". Jon didn't like the idea of something that he feels duplicates the 503 with a retry after header field. He was on a mission to reduce the number of geolocation-error codes, and these two go whacked. That's where this text comes from. I'll get with him about writing this section better - or do you have an alternate plan we should pursue?
> 
> James

---

From: "James M. Polk" <[email-masked]>
Date: Tue, 29 Mar 2011 07:21:52 -0500

At 06:59 AM 3/29/2011, Robert Sparks wrote:
>Thanks for the reminder - I reread the text and talked with Jon. I 
>think the only thing that needs to change
>is the actual error code. 503 is to extreme - it would cut off all 
>SIP traffic from the peer receiving the response,
>not just traffic related to this request.

yes - that would be bad.

>I think the right code in this context is 500 (which can also use 
>the Retry-After mechanism).
>

got it

Thanks

James

> >> 3) I can't figure out what the paragraph in section 4.4 that 
> starts "Additionally, if a sip entity cannot..." and goes
> > pick on this one point, there was an error code for that 
> basically said "couldn't process the location you just sent me, but 
> it's ok to try again" *and* "couldn't process the location you just 
> sent me, don't try to resend it for a while". Jon didn't like the 
> idea of something that he feels duplicates the 503 with a retry 
> after header field. He was on a mission to reduce the number of 
> geolocation-error codes, and these two go whacked. That's where 
> this text comes from. I'll get with him about writing this section 
> better - or do you have an alternate plan we should pursue?
> >
> > James