sip_transport: tcp: Notify app if unable to keep decoding stream

[pespin]

If pjsip_find_msg() starts failing because there's some missing header like Content-Length, log the issue and shutdown the transport to avoid staying in a broken state.

[CLAassistant]

All committers have signed the CLA.

[trengginas]

Shouldn't calling the callback be enough? The user will be notified and shutdown the transport if needed.

[pespin]

@trengginas I'm fine with your proposal. TBH I care more about the "logging" side of things to start with, to at least report that the stream fails to parse and becomes broken. Before that, there was no message at all, everything happened silently.

I'll submit a new version dropping the transport shutdown.

[pespin]

@trengginas I added the transport_shutdown in the new extended code path because I saw in the other code path it was already being done. So not sure if then this path was wrong then?
Let me know whether you want to merge as it, drop the pjsip_transport_shutdown() from the new path or from both paths.

[nanangizz]

I also vote for callback approach, shutting down transport may not be preferred in some cases so IMO better leave it for app to decide.

[pespin]

I just noticed I actually forgot to update the commit description. I did it now, so I think it's now ready to be merged.

[nanangizz]

Hmm.. honestly the flow resturcturing make me nervous about possibility of bug or major behavior change.

[pespin]

@nanangizz it's just a matter of looking at pjsip_find_msg() and seeing the error codes returned in each scenario, and act on them.

The whole "if (msg_status != PJ_SUCCESS) {" path is transformed in the first case so we get rid of a whole indentation level.

Then, the other msg_status are further discriminated. I'm keeping the old code path (the one returning PJSIP_EPARTIALMSG) the same. I'm simply extending the other error codes to handle them, since they were ignored in the past.

[nanangizz]

@nanangizz it's just a matter of looking at pjsip_find_msg() and seeing the error codes returned in each scenario, and act on them.

Originally non-success cases are evaluated based on remaining_len instead of error codes. To me this is not "just a matter of ...", it definitely needs a very careful check to make sure no behavior change is introduced. Note that this code is part of the core of the lib and have been used and tested for a long time. This is what makes me nervous.

The whole "if (msg_status != PJ_SUCCESS) {" path is transformed in the first case so we get rid of a whole indentation level.

Indentation is not a very strong reason to modify the flow, the code is only few lines and not that hard to read. CMIIW the new proposed feature in this PR seems to be able to be inserted with much less risk and easier to review :)