Opened 16 years ago
Closed 13 years ago
#2194 defect closed fixed (fixed)
small bug in SIP Via header generation
Reported by: | Antoine Pitrou | Owned by: | washort |
---|---|---|---|
Priority: | highest | Milestone: | |
Component: | core | Keywords: | |
Cc: | Branch: |
branches/via-parameters-2194-2
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | washort |
Description
There is a small bug in the Via.toString() method. Perhaps it can only occur in theoretical conditions (I don't know enough about the SIP protocol to know for sure).
However, here it is:
>>> from twisted.protocols import sip >>> via = sip.Via("localhost", ttl=2) >>> via.toString() 'SIP/2.0/UDP localhost:5060;ttl=2' >>> via = sip.Via("localhost", ttl=1) >>> via.toString() 'SIP/2.0/UDP localhost:5060;ttl'
As you see, if a value is exactly 1, it isn't output. This is because of the test "if value == True
" at line 207. Replacing it with "if value is True
" fixes the bug.
Change History (15)
comment:1 Changed 16 years ago by
comment:3 Changed 16 years ago by
The bug can happen with any parameter, I just took "ttl" as an example. Also, RFC 3261 says (section 18.2.2):
If the address is a multicast address, the response SHOULD be sent using the TTL indicated in the "ttl" parameter, or with a TTL of 1 if that parameter is not present.
It does not say there is a default value if the parameter is present but does not specify a value.
comment:4 Changed 16 years ago by
Owner: | changed from Glyph to washort |
---|
comment:5 Changed 13 years ago by
Author: | → washort |
---|---|
Branch: | → branches/via-parameters-2194 |
(In [25676]) Branching to 'via-parameters-2194'
comment:6 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | washort deleted |
comment:7 Changed 13 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:8 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to washort |
Status: | assigned → new |
- twisted/protocols/sip.py
- In the class docstring for
Via
several attributes are documented as Optional or Required. I see that this is information from the spec, but I'm not sure what it means for the attribute. This should be converted into information which can more easily be used by a developer likely to interact with this class (for example, perhaps the optional attributes should be documented as possibly having aNone
value if the information isn't present). - The types of most of the attributes are not documented but should be.
- The enormous try/except in
parseViaHeader
doesn't seem ideal to me. One specific problem it has is that some of the parsing code raisesSIPError
with detailed information about how the parsing failed (the wrong version number case), but the except suite squashes everything into a generic SIPError. Another specific problem is thelog.err
call in the except suite; low-level parsing functions shouldn't log errors, they should raise exceptions indicating how parsing failed. Another problem is that there seems to be no test coverage for the exception handling in this function; it's hard to really decide if the implementation strategy is appropriate without tests. :)
- In the class docstring for
- The test suite hangs at
testAmoralRPort
for me. A parse error shows up in the logs. This may indicate that the Via rport handling has changed incompatibly... - Why was
testExtraWhitespace
deleted?
comment:9 Changed 13 years ago by
Branch: | branches/via-parameters-2194 → branches/via-parameters-2194-2 |
---|
(In [26100]) Branching to 'via-parameters-2194-2'
comment:10 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | washort deleted |
Priority: | normal → highest |
Fixed. The try/except turned out to be useless since nothing catches any exceptions at the call sites for parseViaHeader.
comment:11 Changed 13 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:12 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to dash |
Status: | assigned → new |
- It seems that there's still an incompatibility in the
rport
change. Old code passingTrue
will get inexpected behavior (rport=True, I think). Can you deprecate this use of theVia.__init__
rport
parameter and restore the old behavior for that case? - Before
testExtraWhitespace
was deleted. Now it's duplicated. Er? - I don't think
_Absent.__nonzero__
quite does the trick. Even if the deprecated code isn't being edited, its behavior is potentially being changed. That's fine, if it has tests, but it doesn't appear to have tests. So it probably needs to have tests added. It also looks like some of it is broken by this change - for example,Base._fixupNAT
compares the value ofrport
toTrue
. That case never happens anymore, right? It's now theNone
case.
comment:13 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | dash deleted |
OK, new scheme for compatibility added (and other stuff fixed).
comment:14 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | set to washort |
- flakes: Not your fault, but in
test_sip
, twisted.cred.checkers and twisted.cred.portal are apparently unused. Also,twisted.cred.error
is imported in a weird way. getWarningMethod()
should really be deprecated. It's no longer necessary. just usewarnings.warn
.BAD_REQUEST
is apparently never used. Neither is a "400" used anywhere in the diff. I guess it should just be deleted?- I think it would be clearer to call the getter
_getrport
, rather than the ambiguousrport
. (Also the setter should be_setrport
, since I find the case difference is a bit confusing.)- also please two blank lines after the
rport
method, when you change its name.
- also please two blank lines after the
- Given that
rportRequested
is a boolean, the "== True" crud is just noise. Delete it. - Docstrings should use
L{}
more often. For example, instead of using a sentence fragment,Via
's docstring could begin with: "A L{Via} is a SIP 'Via' header...".parseViaHeader
should say "Parse a 'via' header, returning a L{Via}." Also it could say "@return" and "@rtype".
This is all pretty cosmetic, so please address and then merge.
comment:15 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
And here is a test to be added in test_sip:ViaTestCase.