[Asterisk-code-review] chan_sip: in case of tcp/tls, be less annoying about tx errors. (asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13366 )

Change subject: chan_sip:  in case of tcp/tls, be less annoying about tx errors.
......................................................................


Patch Set 1:

> 1.  if (!tcptls_session) .... obviously we can't write if we don't have a connection - so keep quiet about it now and if it's a reliable transmission hope the peer reconnects before it times out (nothing better we can do).
> 2.  The code path you mention is more complex, there are four possible failure modes:
> 2.1 fd == -1 - I'm assuming no connection - whould probably be treated like 1 above.
> 2.2 issues getting some thread - doesn't look like ao2_t_find will print an error ...
> 2.3 ao2_alloc error for packet; and 
> 2.4 packet string alloc error - both presumably memory errors.  Not sure if they'll get logged.
> 
> I suspect the four paths for 2 should be split up, and more detailed logs be printed for each sub-branch, before going to the tcptls_write_setup_error label.
> 
> If you're happy with that as a solution, it'll end up that:
> 
> 1 and 2.1 never gets logged.  It'll be like sending a udp frame that gets lost on the wire somewhere.
> 2.2 will be logged as "Unable to locate tcptls helper thread"
> 2.3 and 2.4 will get logged as "memory allocation failure"
> 
> Happy?

I think your proprosal is acceptable. #1 was already essentially not logged in this case as if it's NULL the function never gets called. Which brings up the point that this function is called from at least one other location. It should be fine, but I'd suggest double checking to make sure any added, or removed logging makes sense for that call scenario as well.


-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13366
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I46eb82924beeff9dfd86fa6c7eb87d2651b950f2
Gerrit-Change-Number: 13366
Gerrit-PatchSet: 1
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 05 Dec 2019 19:06:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191205/415e8dec/attachment.html>