sipsess: add cancel reason to cancel handler #1309
Conversation
|
Hi, sorry that it took a few commits more than I'd have liked to get this right, it's my first time contributing to a C project. The CI is now green on my fork (besides the abicheck (because tags aren't transferred when forking but that depends on the version tag being present) and the sonar workflow (because I don't have a sonar token)). |
| @@ -275,7 +275,7 @@ typedef int(sip_send_h)(enum sip_transp tp, struct sa *src, | |||
| typedef int(sip_conn_h)(struct sa *src, const struct sa *dst, struct mbuf *mb, | |||
| void *arg); | |||
| typedef void(sip_resp_h)(int err, const struct sip_msg *msg, void *arg); | |||
| typedef void(sip_cancel_h)(void *arg); | |||
| typedef void(sip_cancel_h)(const struct sip_msg *msg, void *arg); | |||
There was a problem hiding this comment.
Is it possible to implement this without breaking the API ?
There was a problem hiding this comment.
I could attach the information about the cancellation to the struct sipsess which is passed as the void *arg there. But this isn't the only place the API would have a breaking change. Passing the cancel handlers from the upper layer is also a breaking change and I don't see how that can be avoided.
There was a problem hiding this comment.
no problem, it should be fine to break the API if that is the only option ...
We try to avoid breaking the API, but sometimes you have to do it ...
The void pointer to arg is supposed to be opaque, so that this layer has no
compile-time knowledge about the upper layer.
Another option is to save the Cancel Reason on the SIP-state itself and add
a getter function, something like this:
struct {
...
uint16_t cause;
char *text;
....
};
void sip_get_cancel_reason(struct sip *sip, uint16_t *causep, char **textp);There was a problem hiding this comment.
Yep that would also work, although I would attach it to the struct sip_trans since that's where the cancel handler and the void *arg is stored.
Do you think that would be better? I think passing the message in the cancel handler is the more obvious way. The response handler does this the same way to communicate the related information (like errors or similar) to the upper layer.
There was a problem hiding this comment.
either way is fine with me ..
|
Could you please also add a copy of your SIP signalling, for reference ? |
As mentioned in the feature request, the important line here is this: |
| @@ -30,6 +30,7 @@ typedef void (sipsess_info_h)(struct sip *sip, const struct sip_msg *msg, | |||
| typedef void (sipsess_refer_h)(struct sip *sip, const struct sip_msg *msg, | |||
| void *arg); | |||
| typedef void (sipsess_close_h)(int err, const struct sip_msg *msg, void *arg); | |||
| typedef void (sipsess_cancel_h)(const struct sip_msg *msg, void *arg); | |||
There was a problem hiding this comment.
can you see if you can reuse the close handler here ?
if there is an incoming CANCEL message, the close handler should be called ...
There was a problem hiding this comment.
They way this currently works is that:
- a cancel message comes in
- the current sip_cancel_h is called (which is always this static function in accpect.c
static void cancel_handler(void *arg)
{
struct sipsess *sess = arg;
(void)sip_treply(&sess->st, sess->sip, sess->msg,
487, "Request Terminated");
sess->peerterm = true;
if (sess->terminated)
return;
sipsess_terminate(sess, ECONNRESET, NULL);
}- This calls sipsess_terminate
void sipsess_terminate(struct sipsess *sess, int err,
const struct sip_msg *msg)
{
sipsess_close_h *closeh;
void *arg;
if (sess->terminated)
return;
closeh = sess->closeh;
arg = sess->arg;
if (!termwait(sess))
(void)terminate(sess);
closeh(err, msg, arg);
}Note that sipsess_close_h is called here, but the msg is passed in as NULL. I rectified that by changing the signatures of the cancel_handler function and the sip_cancel_h, which is one of the breaking changes.
Afaict this means that the message isn't passed to the close handler, and afaict the session close handler is not called anywhere else.
The only chance I am seeing to avoid any breaking changes is to alter the callsite of the sip_cancel_h here: to call the sip_resp_h with the sip_msg *msg and to somehow call the sipsess_close_h from there.
But that seems like a pretty ugly way of doing things to me for several reasons:
- Cancelling a is a pretty important interaction that is quite different from just closing the connection
- Hijacking the response handler seems weird, because the cancel isn't a response, it's an additional request that cancels a previous request
- This would still change the API just not in a compile breaking way. We add some important semantics to the close handler,
- and either we call closeh twice (once from the cancel handler with a NULL message and once from the response handler)
- or we need to take care of only calling the handler once which seems prone to producing subtle bugs in future
Other than that we can maybe avoid the sipsess_cancelh by reusing the sipsess_close_h but that would still need the breaking change to the sip_cancel_h so we can have the sip_msg in the cancel handler to pass to sipsess_terminate and in turn to the sipsess_close_h. And if we are already breaking the API anyways I'd favor not hacking support for cancellation into the close handler but to have a separate cancel handler passed from the upper layer.
There was a problem hiding this comment.
Or, if your question was whether I could use the sipsess_close_h type for cancel handlers: Yeah sure. But why? The cancel handlers are new function args which is a breaking change in any case. Let's give them an appropriate type, right?
There was a problem hiding this comment.
After more thought: I guess I can attach a close_message to the struct sip_trans and pass that to the close handler in terminate_session
I want to display the provided reason in Cancel messages to the user of baresip, but as far as I could tell the Cancel messages are not communicated outside of libre. So I added a cancel handler, very similar to all the other handlers.
This is a breaking API change though, so I am not sure if this is the best way to go about this.
Related to this feature request: baresip/baresip#3363