| Summary: | ASTERISK-10749: bugs in udptl.c | ||
| Reporter: | cache (cache) | Labels: | |
| Date Opened: | 2007-11-13 03:48:35.000-0600 | Date Closed: | 2007-12-06 10:48:30.000-0600 |
| Priority: | Minor | Regression? | No |
| Status: | Closed/Complete | Components: | Channels/chan_sip/T.38 |
| Versions: | Frequency of Occurrence | ||
| Related Issues: | |||
| Environment: | Attachments: | ( 0) udptl-2.patch ( 1) udptl-3.patch ( 2) udptl-4.patch ( 3) updtl.diff | |
| Description: | While working on T38 terminal support for asterisk I discovered I can not properly talk to Calweaver via UDPTL. It was strange because it seem like their UDPTL implementation have common roots. I found that for some reason two lines of udptl.c are commented out causing the problem. Attached patch fixes the issue | ||
| Comments: | By: Dmitry Andrianov (dimas) 2007-11-13 17:33:13.000-0600 I can't see Cache's patch until he disclaims it, but from a private chat with him I assume he fixed line 372+392 in the udptl.c Unfortunately it is not enough because this only fixes IFP sequence numbering for "Secondary packet mode error recovery" and does not work in FEC mode. Attached patch which fixes both cases and also does couple of cleanups: 1. moves common code out of if/else 2. makes "Got UDPTL" messages be displayed exactly under same conditions as "Sent UDPTL" to avoid confusion By: Olle Johansson (oej) 2007-11-15 04:53:36.000-0600 This is more than two lines of changes. Is this really your own code, all of it? (just checking the license since you mention you compare with callweaver). By: Olle Johansson (oej) 2007-11-15 04:54:51.000-0600 This propably applies to 1.4 as well. By: cache (cache) 2007-11-15 05:10:17.000-0600 Callwaever is being used only as fax-machine, and not for review it code. Therefore at license side all is clear. By: Olle Johansson (oej) 2007-11-15 05:19:22.000-0600 Ok, thanks for making that clear so quickly! /O By: Dmitry Andrianov (dimas) 2007-11-15 05:32:48.000-0600 And actually, there is no "our" code. Basically, what was done: 1. uncommented several lines. I think they were commented out because when udptl.c was initially added, ast_frame structure missed seqno field or something - I think so because commented lines contained "???" before each frame.seq_no reference 2. code common for "if" and "else" branches moved out of "if" just to save space. Still, this code was already present in the file - it was just moved. By: Dmitry Andrianov (dimas) 2007-11-15 06:55:11.000-0600 A bit more fixes. While debugging UDPTL I found that seq numbers in "Sent UDPTL" log messages are bogus. It appears that ast_udptl has unused field which is initialized with random value and it is only used for logging instead of proper (tx_seq_no) field. udptl-3.patch fixes that (the patch also contains all fixes from udptl-2.patch) By: Dmitry Andrianov (dimas) 2007-11-15 08:23:23.000-0600 Sorry, more fixes :) 1. udptl_rx_packet could return success even if it did not retrieved any IFP (because it already seen that seqno) 2. udptl_read did not check return value of udptl_rx_packet so even if there were parse error, udptl_read considers everything is ok Due to these issues, udptl_read was generating strange frames with frametype=0, subclass=0. Patch makes it generate null_frame in these cases. (udptl-4.patch also includes all previous patches) By: Digium Subversion (svnbot) 2007-12-06 10:47:02.000-0600 Repository: asterisk Revision: 91450 U branches/1.4/main/udptl.c ------------------------------------------------------------------------ r91450 | file | 2007-12-06 10:47:01 -0600 (Thu, 06 Dec 2007) | 6 lines Fix various in the udptl implementation. It could return empty modem frames, have an incorrect sequence number on packets, and display the wrong sequence number in the debug messages. (closes issue ASTERISK-10749) Reported by: Cache Patches: udptl-4.patch uploaded by dimas (license 88) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=91450 By: Digium Subversion (svnbot) 2007-12-06 10:48:29.000-0600 Repository: asterisk Revision: 91458 _U trunk/ U trunk/main/udptl.c ------------------------------------------------------------------------ r91458 | file | 2007-12-06 10:48:29 -0600 (Thu, 06 Dec 2007) | 14 lines Merged revisions 91450 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r91450 | file | 2007-12-06 12:49:42 -0400 (Thu, 06 Dec 2007) | 6 lines Fix various in the udptl implementation. It could return empty modem frames, have an incorrect sequence number on packets, and display the wrong sequence number in the debug messages. (closes issue ASTERISK-10749) Reported by: Cache Patches: udptl-4.patch uploaded by dimas (license 88) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=91458 | ||