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.

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

Additionally return reason header protocol and cause #2935

Conversation

ajsa-terko
Copy link
Contributor

Current syntax for reporting on SIP code, reason, and reason header looks as:

{
        "sip" : "event",
        "result" : {
                "event" : "<name of the error event>",
                "code" : <SIP error code>,
                "reason" : "<SIP error reason>",
                "reason_header" : "<SIP reason header; optional>"
        }
}

reason_header, although its name suggests differently, returns only the value of the text parameter from the reason header, (correspond to the value of the text parameter returned by Sofia, re_text). Looking at the example below, the current response would return "reason_header": "Terminated".

Reason: Q.850;cause=16;text=”Terminated”

As an additional two parameters from the common syntax of the header could provide more context to what has happened (and enhance reason header mappings on the user side), the pull request suggests adding two new optional fields, reason_header_protocol, and reason_header_cause. Both of these are contained in Sofia's sip reason structure sip_reason_s as re_protocol and re_cause, and are read and returned in the same way as the current reason_header.

After the update, the new syntax would be as:

{
        "sip" : "event",
        "result" : {
                "event" : "<name of the error event>",
                "code" : <SIP error code>,
                "reason" : "<SIP error reason>",
		"reason_header" : "<Reason header; optional>",
		"reason_header_protocol" : "<Reason header protocol; optional>",
		"reason_header_cause" : "<Reason header cause code; optional>"
        }
}

I would appreciate feedback on naming both of these fields, as reason_header may be misleading having protocol and cause separately, but cannot be simply renamed not to introduce breaking change.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

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

I don't use the SIP plugin much, but I think it makes sense, especially in case those properties are indeed of use to users/developers. I agree that renaming them all would be better (possibly grouped in an object) but we do indeed have to preserve backwards compatibility, so I have no opinion on the names of the properties.

@@ -7157,4 +7195,4 @@ gpointer janus_sip_sofia_thread(gpointer user_data) {
JANUS_LOG(LOG_VERB, "Leaving sofia loop thread...\n");
g_thread_unref(g_thread_self());
return NULL;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like your editor ate the last newline?

Copy link
Contributor Author

@ajsa-terko ajsa-terko Mar 31, 2022

Choose a reason for hiding this comment

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

Indeed, it's back now

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@ajsa-terko ajsa-terko requested a review from lminiero April 3, 2022 20:08
@lminiero
Copy link
Member

lminiero commented Apr 4, 2022

Thanks for the fix, I'll make a couple of tests and, if everything is fine (which I expect it to be), I'll merge 👍

@lminiero
Copy link
Member

lminiero commented Apr 4, 2022

Looks good here: I'll merge and backport the same fix to the 0.x branch as well.

@lminiero lminiero merged commit 5c3e9cb into meetecho:master Apr 4, 2022
lminiero added a commit that referenced this pull request Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants