Re: Second WG last-call for SIP/SigComp - dedicated review
Gonzalo Camarillo <Gonzalo.Camarillo <at> ericsson.com>
2007-01-19 10:59:56 GMT
Hi,
thanks for your review. Answers inline:
> Abstract and introduction:
> "Any implementation of SigComp for use with SIP must confirm to this
> document."
> This sentence appears in the introduction too. What does this mean
> without the must being a MUST (I realise it shouldn't be a MUST in the
> abstract)? What does this mean for legacy implementations /
> implementations conforming to other specifications e.g. 3GPP TS24.229?
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.
> Introduction:
> "Additionally, it must support ...[3485] and ...[3486]."
> Again no standards language. Support for 3485 is specified further on
> in the document (so I think isn't needed here) but support for 3486
> isn't so what does this mean for support of that RFC?
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).
> 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'.
I will be happy to add the clarifying sentence you request if you
provide me with it.
> Section 3.2:
> The arguments now is:
> * 'non-zero SMS removes uncertainty' - that's not strictly true because
> the application can still refuse state creation.
> * Stateless SIP implementations are unlikely to do SigComp - fair
> enough
> * 2K per compartment isn't a huge overhead for a stateful
> implementation - ok
> [12/12/06] Some text has been removed from this section which is fine,
> but the first point still seems slightly disingenuous and could cause
> confusion.
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.
^^^^^^
> 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."
> 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.
> 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.
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.
> 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.
> 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.
> 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.
> 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.
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.
> 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 3: page 3
> "Note: a SigComp endpoint MAY offer additional resources if available;"
> - make clear that it's additional over and above the minimum values
> specified in this document rather than the basic SigComp minimum values.
Fixed.
> Section 4: second paragraph:
>
> "For a message-based transport such as UDP or SCTP, this can be done per
> message."
> ->
> "For a message-based transport such as UDP or SCTP, distinguishing
> between SigComp and non-SigComp messages can be done per message."
Fixed.
> Section 4: third paragraph:
>
> "For a stream-based transport such as TCP, this can be done per
> connection."
> ->
> "For a message-based transport such as UDP or SCTP, distinguishing
> between SigComp and non-SigComp has to be done per connection."
Fixed (taking into account the copy/paste error in your email).
> 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.
>
> 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.
> Section 8.1:
> "Incoming responses: the remote application identifier is the same as
> the one of the previously-sent request that initiated the transaction
> the response belongs to."
> ->
> "Incoming responses: the remote application identifier is the same as
> that of the previously-sent request that initiated the transaction to
> which the response belongs."
Fixed.
>
> "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.
> comparment -> compartment (1 instance in section 8.3 and 1 in section
> 11)
Fixed.
>
> Section 8.4: last paragraph
> "If following the previous rules" -> "If, following the rules above, "
Fixed.
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
RSS Feed