Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sipsess: refactor and simplify SDP negotiation state #1016

Merged
merged 1 commit into from
Jan 2, 2024
Merged

sipsess: refactor and simplify SDP negotiation state #1016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions include/re_sipsess.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,15 @@
struct sipsess_sock;
struct sipsess;

/* SDP Negotiation state */
enum sdp_neg_state {
SDP_NEG_NONE = 0,
SDP_NEG_LOCAL_OFFER, /** SDP offer sent */
SDP_NEG_REMOTE_OFFER, /** SDP offer received */
SDP_NEG_PREVIEW_ANSWER, /** SDP preview answer sent */
SDP_NEG_DONE /** SDP negotiation done */
};
sreimers marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a "state" or "flags" ?

If it is flags then the stored value can contain a bitmask of many values ORed together.

If it is a state, the value can only contain one value and the states values 1<<N is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I thought it could be useful in some cases to OR the values (like flags), but I didn't end up using it anywhere. I have removed the explicit enum values.



typedef void (sipsess_conn_h)(const struct sip_msg *msg, void *arg);
typedef int (sipsess_desc_h)(struct mbuf **descp, const struct sa *src,
Expand Down Expand Up @@ -74,3 +83,4 @@ void sipsess_close_all(struct sipsess_sock *sock);
struct sip_dialog *sipsess_dialog(const struct sipsess *sess);
void sipsess_abort(struct sipsess *sess);
bool sipsess_ack_pending(const struct sipsess *sess);
enum sdp_neg_state sipsess_sdp_neg_state(const struct sipsess *sess);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the state have to be exposed to the application?

Copy link
Contributor Author

@maximilianfridrich maximilianfridrich Nov 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so. E.g. the application can check if the state is SDP_NEG_PREVIEW_ANSWER (unreliable SDP answer was sent in 1XX response) and in this case it MUST send the exact same SDP in the 200 OK. Ideally, re would not allow non RFC conformant usage, but we're not there yet. That would take more refactoring that could be done in a subsequent step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following BareSIP PR makes use of the state:

3 changes: 3 additions & 0 deletions src/sipsess/accept.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ int sipsess_accept(struct sipsess **sessp, struct sipsess_sock *sock,
if (err)
goto out;

if (mbuf_get_left(msg->mb))
sess->neg_state = SDP_NEG_REMOTE_OFFER;

va_start(ap, fmt);

if (scode > 100 && scode < 200) {
Expand Down
70 changes: 50 additions & 20 deletions src/sipsess/connect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ static int send_handler(enum sip_transp tp, struct sa *src,
*contp = cont;

out:
sess->sent_offer = desc != NULL;
if (desc)
sess->neg_state = SDP_NEG_LOCAL_OFFER;

mem_deref(desc);
return err;
}
Expand All @@ -89,20 +91,14 @@ static void invite_resp_handler(int err, const struct sip_msg *msg, void *arg)
if (!msg || err || sip_request_loops(&sess->ls, msg->scode))
goto out;

sdp = mbuf_get_left(msg->mb) > 0;

if (msg->scode < 200) {
sess->progrh(msg, sess->arg);
sdp = mbuf_get_left(msg->mb) > 0;

if (msg->scode == 100)
return;

if (sdp && sess->sent_offer) {
sess->awaiting_answer = false;
err = sess->answerh(msg, sess->arg);
if (err)
goto out;
}

contact = sip_msg_hdr(msg, SIP_HDR_CONTACT);
if (pl_isset(&msg->to.tag) && contact) {
err = sip_dialog_established(sess->dlg) ?
Expand All @@ -112,19 +108,40 @@ static void invite_resp_handler(int err, const struct sip_msg *msg, void *arg)
goto out;
}

if (sdp && sess->neg_state == SDP_NEG_LOCAL_OFFER) {
err = sess->answerh(msg, sess->arg);
if (err)
goto out;
}

if (sip_msg_hdr_has_value(msg, SIP_HDR_REQUIRE, "100rel")
&& sess->rel100_supported) {
if (sdp && !sess->sent_offer) {
sess->modify_pending = false;
err = sess->offerh(&desc, msg, sess->arg);

if (sess->neg_state == SDP_NEG_NONE && !sdp)
goto out;

if (sdp) {
if (sess->neg_state == SDP_NEG_LOCAL_OFFER) {
sess->neg_state = SDP_NEG_DONE;
}
else if (sess->neg_state == SDP_NEG_NONE) {
sess->neg_state = SDP_NEG_REMOTE_OFFER;
err = sess->offerh(&desc, msg,
sess->arg);
}
}

err |= sipsess_prack(sess, msg->cseq.num, msg->rel_seq,
&msg->cseq.met, desc);
mem_deref(desc);
sess->desc = mem_deref(sess->desc);
if (err)
goto out;

if (sess->neg_state == SDP_NEG_REMOTE_OFFER
&& mbuf_get_left(desc))
sess->neg_state = SDP_NEG_DONE;

mem_deref(desc);
sess->desc = mem_deref(sess->desc);
}

return;
Expand All @@ -139,15 +156,28 @@ static void invite_resp_handler(int err, const struct sip_msg *msg, void *arg)
if (err)
goto out;

if (sess->sent_offer)
err = sess->answerh(msg, sess->arg);
else {
sess->modify_pending = false;
err = sess->offerh(&desc, msg, sess->arg);
if (sdp) {
if (sess->neg_state == SDP_NEG_LOCAL_OFFER) {
sess->neg_state = SDP_NEG_DONE;
err = sess->answerh(msg, sess->arg);
}
else if (sess->neg_state == SDP_NEG_NONE) {
sess->neg_state = SDP_NEG_REMOTE_OFFER;
err = sess->offerh(&desc, msg, sess->arg);
}
}

err |= sipsess_ack(sess->sock, sess->dlg, msg->cseq.num,
sess->auth, sess->ctype, desc);
sess->auth, sess->ctype, desc);
if (err)
goto out;

if (sess->neg_state == SDP_NEG_NONE && !sdp)
goto out;

if (sess->neg_state == SDP_NEG_REMOTE_OFFER
&& mbuf_get_left(desc))
sess->neg_state = SDP_NEG_DONE;

sess->established = true;
mem_deref(desc);
Expand Down
55 changes: 34 additions & 21 deletions src/sipsess/listen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,13 @@ static void bye_handler(struct sipsess_sock *sock, const struct sip_msg *msg)
static void ack_handler(struct sipsess_sock *sock, const struct sip_msg *msg)
{
struct sipsess *sess;
bool awaiting_answer;
int err = 0;

sess = sipsess_find(sock, msg);
if (!sess)
return;

if (sipsess_reply_ack(sess, msg, &awaiting_answer))
if (sipsess_reply_ack(sess, msg))
return;

if (sess->terminated) {
Expand All @@ -146,8 +145,13 @@ static void ack_handler(struct sipsess_sock *sock, const struct sip_msg *msg)
return;
}

if (awaiting_answer) {
sess->awaiting_answer = false;
if (sess->neg_state == SDP_NEG_LOCAL_OFFER) {
if (!mbuf_get_left(msg->mb)) {
sipsess_terminate(sess, EPROTO, NULL);
return;
}
maximilianfridrich marked this conversation as resolved.
Show resolved Hide resolved

sess->neg_state = SDP_NEG_DONE;
err = sess->answerh(msg, sess->arg);
}

Expand All @@ -169,15 +173,14 @@ static void ack_handler(struct sipsess_sock *sock, const struct sip_msg *msg)

static void prack_handler(struct sipsess_sock *sock, const struct sip_msg *msg)
{
bool sdp;
struct sipsess *sess;
struct mbuf *desc = NULL;
bool awaiting_answer = false;
bool awaiting_prack = false;

sess = sipsess_find(sock, msg);

if (!sess || sipsess_reply_prack(sess, msg, &awaiting_answer,
&awaiting_prack)) {
if (!sess || sipsess_reply_prack(sess, msg, &awaiting_prack)) {
(void)sip_reply(sock->sip, msg, 481,
"Transaction Does Not Exist");
return;
Expand All @@ -192,22 +195,28 @@ static void prack_handler(struct sipsess_sock *sock, const struct sip_msg *msg)
return;
}

if (awaiting_prack) {
sess->awaiting_prack = false;
sess->refresh_allowed = true;
}
sdp = mbuf_get_left(msg->mb);

if (sess->prackh)
sess->prackh(msg, sess->arg);
if (awaiting_prack)
--sess->prack_waiting_cnt;
maximilianfridrich marked this conversation as resolved.
Show resolved Hide resolved

if (awaiting_answer) {
sess->awaiting_answer = false;
if (sess->neg_state == SDP_NEG_LOCAL_OFFER) {
if (!sdp) {
sipsess_terminate(sess, EPROTO, NULL);
return;
}
maximilianfridrich marked this conversation as resolved.
Show resolved Hide resolved

sess->neg_state = SDP_NEG_DONE;
(void)sess->answerh(msg, sess->arg);
}
else if (msg && mbuf_get_left(msg->mb)) {
else if (sess->neg_state == SDP_NEG_DONE && sdp) {
sess->neg_state = SDP_NEG_REMOTE_OFFER;
(void)sess->offerh(&desc, msg, sess->arg);
}

if (sess->prackh)
sess->prackh(msg, sess->arg);

(void)sipsess_reply_2xx(sess, msg, 200, "OK", desc, NULL, NULL);

mem_deref(desc);
Expand All @@ -219,7 +228,7 @@ static void target_refresh_handler(struct sipsess_sock *sock,
{
struct sip *sip = sock->sip;
bool is_invite;
bool got_offer;
bool sdp;
struct sipsess *sess;
struct mbuf *desc = NULL;
char m[256];
Expand All @@ -232,14 +241,15 @@ static void target_refresh_handler(struct sipsess_sock *sock,
}

is_invite = !pl_strcmp(&msg->met, "INVITE");
got_offer = (mbuf_get_left(msg->mb) > 0);
sdp = (mbuf_get_left(msg->mb) > 0);

if (!sip_dialog_rseq_valid(sess->dlg, msg)) {
(void)sip_treply(NULL, sip, msg, 500, "Server Internal Error");
return;
}

if ((is_invite && sess->st) || sess->awaiting_answer) {
if ((is_invite && sess->st)
|| (sdp && sess->neg_state == SDP_NEG_LOCAL_OFFER)) {
(void)sip_treplyf(NULL, NULL, sip, msg, false,
500, "Server Internal Error",
"Retry-After: 5\r\n"
Expand All @@ -253,16 +263,19 @@ static void target_refresh_handler(struct sipsess_sock *sock,
return;
}

if (got_offer && !sipsess_refresh_allowed(sess)) {
if (sdp && !sipsess_refresh_allowed(sess)) {
(void)sip_reply(sip, msg, 488, "Not Acceptable Here");
return;
}

if (is_invite || got_offer) {
if (is_invite || sdp) {
sess->neg_state = sdp ? SDP_NEG_REMOTE_OFFER :
SDP_NEG_LOCAL_OFFER;
err = sess->offerh(&desc, msg, sess->arg);
if (err) {
(void)sip_reply(sip, msg, 488,
str_error(err, m, sizeof(m)));
sess->neg_state = SDP_NEG_DONE;
return;
}
}
Expand Down
Loading
Loading