Subject: [rohc] Re: Second WG last-call for SIP/SigComp - dedicated review

From: Gonzalo Camarillo <[email-masked]>
Date: Sat, 03 Mar 2007 13:43:58 +0200

Hi,

thanks for reviewing the draft again. I have edited the draft in order 
to address all your comments. I will be submitting the new revision 
before the cut off.

Cheers,

Gonzalo

Surtees, Abigail wrote:
> Hi Gonzalo,
> 
> Apologies for the delayed response.  Comments in-line: 
>> yes, abstracts should not have normative language. However, 
>> you are right pointing out that the second 'must' should be 
>> normative. I have moved that text from the Introduction to a 
>> new section right after the Terminology section and I have 
>> made that 'must' a normative MUST.
>>
>> Of course, specifications should be followed once they are published. 
>> Therefore, legacy implementations will, obviously, not be 
>> compliant with this document. That is why the document 
>> contains a Backwards Compatibility section explaining the situation.
>>
>> Regarding implementations confirming to other specs, the 
>> section on Provate Agreements covers that.
>>
> OK
> 
>> we do not use normative language because support for RFC 3485 
>> is already normatively mandated in RFC 3485, and support for 
>> RFC 3586 is already normatively mandated in RFC 3486. We 
>> point to those RFCs, but do not repeat the normative 
>> statements here (this is common practice when writing RFCs).
>>
> 
> OK
> 
>>> Section 3.1:
>>> I know the 'reason' part here but I think it should be written more 
>>> clearly (I think if you were new to it you'd have to read 
>> it several 
>>> times).  Add another sentence to explain the example.  Also, the 
>>> SigComp user guide refers to 'circular buffer' not 'ring 
>> buffer' so it 
>>> may be worth using 'circular buffer' here (ring buffer isn't used 
>>> anywhere else).
>> I have replaced 'ring buffer' with 'circular buffer'.
>>
> OK.
> 
>> I will be happy to add the clarifying sentence you request if 
>> you provide me with it.
>>
> "For example, the
>    condition set by [RFC3320] for SigComp over UDP means: C + 2*B + R +
>    2*S + 128 < DMS (each term is described below).  On the other hand,
>    8KB additional memory should not cause any problem for an endpoint
>    that already implements SIP, SigComp, and applications that use SIP
>    as DMS is memory only temporarily needed during decompression of a
>    SigComp message (the memory can be reclaimed when the message has
>    been decompressed)."
> 
> Rephrased slightly to:
> 
> "For example, the
> condition set by [RFC3320] for SigComp over UDP means: C + 2*B + R +
> 2*S + 128 < DMS (each term is described below), so if DMS is too small
> then at least one of C, B, R and S will be severely restricted.  On the
> other hand, DMS is memory which is only during decompression of a
> SigComp message (after which it can be reclaimed); therefore, a
> requirement of 8Kb should not cause any problem for an endpoint that
> already implements SIP, SigComp, and applications that use SIP."
> 
>>> Section 3.2:
>> I have performed the following change:
>>
>> OLD:
>> Reason: a non-zero SMS allows an endpoint to upload a state in the
>> first SIP message sent to a remote endpoint without the uncertainty of
>> whether or not it can be created in the remote endpoint.
>>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> NEW:
>> Reason: a non-zero SMS allows an endpoint to upload a state in the
>> first SIP message sent to a remote endpoint without the uncertainty of
>> whether the remote endpoint will have enough memory to store such a
>>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> state.
>> ^^^^^^
>>
> OK
> 
>>> Section 3.5:
>>> Add a statement that because the static dictionary is 
>> mandatory it does
>>> not need to be advertised (last point in this email discussion
>>> http://www1.ietf.org/mail-archive/web/rohc/current/msg04463.html).
>> I have added:
>>
>> "Note that, since support for the static SIP/SDP dictionary is
>> mandatory, it does not need to be advertised."
>>
> OK
> 
>>> Section 8.2
>>> Re: Use of IP address and port/connection for the remote application
>>> identifier for legacy systems.  If a client fails (losing 
>> all SIP state
>>> including SigComp compartment) and re-REGISTERS with the same IP
>>> address, how can the SIP stack tell that the old 
>> compartment is invalid.
>>> I realise that the answer is probably that this circumstance is
>>> relatively unlikely and that NACK would recover the situation, but I
>>> think this should be made clearer.
>> Implementors are made aware of this type of problem related to legacy 
>> systems in the section about Backwards Compatibility.
> 
> Hmm, ok.  This one isn't explicitly listed, but the lack of NACK is and
> it's when NACK isn't present that the problem would be more major.
>>> Section 8.3 
>>> I think this section should be combined with section 8.4.  The
>>> definition of when compartments should be opened and closed 
>> needs to be
>>> stronger.  In this section, section 8.4 and section 12, 
>> there are strong
>>> echoes of the fact that it used to be possible to have different
>>> compartment lifetimes.
>>> I think there should be standards language (probably 
>> SHOULDs) stating
>>> when compartments should be opened and closed.  i.e. 
>> compartments SHOULD
>>> last the lifetime of the registration, but leaving the 
>> possibility for
>>> early closure and NACK recovery in extreme cases (e.g. overload
>>> conditions).
>> I have combined Sections 8.3 and 8.4, as you suggest.
> 
> (now 9.3) OK
> 
>> The document already contains normative language regarding when one 
>> should open and close compartments. Regarding the MAY strength on 
>> compartment closure, we need to leave the door open for 
>> implementations 
>> to keep states after the registration expires.
>>
>> With respect to your overload example, the document already says that 
>> implementations SHOULD NOT close the compartments before the 
>> registration expires. Note that we do not use MUST NOT.
> 
> OK
>>> Section 8.3 (Paragraph about keeping state beyond the end of a
>>> compartment):
>>> This paragraph still refers to compartments lasting the length of a
>>> dialog or transaction!
>>> How useful is the technique described in this paragraph, now that a
>>> compartment lifetime is a registration? 
>> I have removed the text talking about dialogs and 
>> transactions. Per my 
>> comment above, we do not need to force implementations to close their 
>> compartments when the registration expires. If they want to 
>> do it, they 
>> can do it, but if they don't, they do not harm anyone by not 
>> closing the 
>> compartment.
>>
> OK
>>> Section 8.4
>>> See comment on section 8.3 about strengthening compartment 
>> definition.
>>> I also think the security implications of opening a compartment on
>>> receipt of a REGISTER should be mentioned.  Compartments 
>> and state can
>>> only be created if the application says so, but, particularly if the
>>> decompressing node is a proxy, the non-2xx response may 
>> take a while to
>>> be generated.
>> Non-2xx responses for REGISTER requests are generated immediately.
> 
> But, if the response is not generated at the proxy, then surely it's
> going to take some time for the response to come back from the server?
> 
>>> Section 8.4
>>> "However, applications MAY also choose to keep these
>>>    compartments open for a longer period of time, as discussed
>>>    previously."
>>> Does the 'as discussed previously refer to the 'paragraph 
>> about keeping
>>> state beyond the end of a compartment'?  If so, then i) I'm not sure
>>> it's that useful and ii) the wording implies something 
>> different.  If
>>> not, then what does it refer to?
>> it refers to the text you point to and, per my comments 
>> above, I believe 
>> it is indeed a useful feature.
>>
> OK
>>> Section 8.5
>>> I was surprised to see this section (but maybe I missed 
>> it's discussion
>>> on the list).  I can understand softening the 'SHOULD' in 
>> RFC3486, but I
>>> don't think it should be a 'SHOULD NOT' - it depends on the 
>> stateless
>>> algorithm in use and the size of the message being 
>> compressed.  With a
>>> stateless algorithm that has short bytecode and a large 
>> message (e.g.
>>> INVITE) it is possible to still achieve non-trivial 
>> compression gain.
>>
>> Theoretically, you are right. Practically, no one intends to do that. 
>> Note that revision 03 of this draft covered every single scenario you 
>> could eventually implement. The conclusion was that the spec needs to 
>> focus on realistic scenarios only; this way, it becomes a 
>> simpler spec. 
> 
> In general, regarding the simpler spec, I agree with you.  However, in
> this particular instance, I don't see the need to restrict the
> implementer in this way.  Granted, it is a SHOULD NOT rather than a MUST
> NOT, but if the implementer can implement a stateless algorithm that
> achieves compression (at least for some messages) why should we
> recommend that they shouldn't do that?  Also, how do you know that in
> practice no-one intends to do that?
> 
> Having looked at where the quoted text appears in RFC 3486, it is not
> related to stateless compression or compression without a compartment.
> So I'm still not sure why this paragraph is in this document.
> 
> If there is really a need for a statement (which I don't think there
> is), then I think it should be no more than:
> 
> "RFC 3486 [RFC 3486] states that:
> 
>       "If the next-hop URI contains the parameter comp=sigcomp, the
>       client SHOULD compress the request using SigComp"
> 
> Although this states that messages SHOULD be compressed, it should be
> noted that in some cases compressing a message may actually result in
> message expansion.  Therefore where comp=sigcomp is specified, it is up
> to the implementation to work out whether or not to compress a message.
> This decision may use available information such as compartment
> availability, and uncompressed and compressed message lengths.
> 
> Note that it is always possible to use the 'well-known shim header'
> mentioned in section 5."
> 
> 
>> That is why revision 04 is much simpler than 03.
>>
>>> Section 12:
>>> I think section 12 should either be part of section 8 or 
>> become section
>>> 9 (preferably the former) because it is an example of compartment
>>> creation and deletion.
>> This is of course a matter of taste, but, as it is the case in many 
>> other RFCs, we have chosen to place the examples at the end 
>> of the document.
> 
> OK, fair enough.
>>> As mentioned above several sentences have echoes of the 
>> possibility of
>>> having variable compartment lifetime:
>>>
>>> "This compartment will be valid, at least, for the
>>>    duration of the registration."
>>> Remove the 'at least'
>>>
>>> "The compartment opened by the outbound proxy will be valid,
>>>    at least, for the duration of the registration."
>>> Remove the 'at least'
>> The 'at least' is there because implementations may choose to 
>> extend the 
>> lifetime of the compartment, per my comments above.
>>
>>> "Since this URI's SIP/SigComp identifier 'Outbound-id' 
>> matches that of
>>>    the compartment created before, this compartment is used 
>> to compress
>>>    the INVITE request."
>>> What other compartment could be used?
>> The node may have SigComp relationships with other nodes at the same 
>> time (e.g., an outbound proxy from other operator for reliability).
>>
>>> Nits:
>>>
>>> Section 4: final note:
>>>
>>> Refers to 'a SigComp structured TCP connection' - I think the word
>>> structured is unnecessary and mildly confusing in this note.
>>> Alternatively make it 'SigComp-structured'
>> Fixed.
> 
> Two occurences, only one is fixed.
>>> Section 7: 
>>> Phraseology 'failure occurred to' and 'loss occurred to' 
>> feel slightly
>>> clumsy.
>>> Rephrase to:
>>> 'failure occurred on' and 'If the response was lost, ...'
>> Fixed.
> There's a second 'to' relating back to the failure occurring which
> should also be an 'on'.
> 
> "or on the response to that message"
> 
>>> "Outgoing responses: the remote application identifier is 
>> the same as
>>> the previously-received request that initiated the transaction the
>>> response belongs to."
>>> ->
>>> "Outgoing responses: the remote application identifier is 
>> the same as
>>> that of the previously-received request that initiated the 
>> transaction
>>> to which the response belongs."
>> Fixed.
> 
> Cut and paste error - during the fix the '>' have crept into the draft!
> 
>>> comparment -> compartment (1 instance in section 8.3 and 1 
>> in section
>>> 11)
>> Fixed.
> 
> Still one instance of comparment in section 12.
>  
>>> Section 8.4: last paragraph
>>> "If following the previous rules" -> "If, following the 
>> rules above, "
>>
>> Fixed.
> 
> Missing 'the'
> 
> Section 13: first paragrpaph under figure 1:
> 
> "'outbound's proxy" -> "outbound proxy's"
> 
> Section 13: page 15, paragraph 4:
> 
> "   The registrar also constructs the route future outgoing requests
>    (from the user agent) and places it in a Service-Route header field
>    in a 200 (OK) response (3). "
> 
> ->
> 
> "   The registrar also constructs the route future outgoing requests
>    (from the user agent) will follow and places it in a Service-Route
> header field
>                          ^^^^^^^^^^^^
>    in a 200 (OK) response (3). "
> 
>> I uploaded a working version of revision 05, which includes all the 
>> changes above, to:
>> http://users.piuha.net/gonzalo/temp/draft-ietf-rohc-sigcomp-sip-05.txt
>>
>> Let me know if you are OK with this version so that I can submit it.
>>
>> Cheers,
>>
>> Gonzalo
>>
> 
> Best regards,
> 
> Abbie

_______________________________________________
Rohc mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/rohc