[Asterisk-code-review] SIP diversion: Fix REDIRECTING(reason) value inconsistencies. (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: SIP diversion: Fix REDIRECTING(reason) value inconsistencies.
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.asterisk.org/#/c/2331/1/res/res_pjsip_diversion.c
File res/res_pjsip_diversion.c:

Line 40: /*!
       :  * \internal
       :  * \brief Determine if the given string is a SIP token.
       :  * \since 13.8.0
       :  *
       :  * \param str String to determine if is a SIP token.
       :  *
       :  * \note A token is defined by RFC3261 Section 25.1
       :  *
       :  * \return Non-zero if the string is a SIP token.
       :  */
       : static int sip_is_token(const char *str)
       : {
       : 	int is_token;
       : 
       : 	if (ast_strlen_zero(str)) {
       : 		/* An empty string is not a token. */
       : 		return 0;
       : 	}
       : 
       : 	is_token = 1;
       : 	do {
       : 		if (!isalnum(*str)
       : 			&& !strchr("-.!%*_+`'~", *str)) {
       : 			/* The character is not allowed in a token. */
       : 			is_token = 0;
       : 			break;
       : 		}
       : 	} while (*++str);
       : 
       : 	return is_token;
       : }
       : 
       : /*! \brief Diversion header reasons
       :  *
       :  * The core defines a bunch of constants used to define
       :  * redirecting reasons. This provides a translation table
       :  * between those and the strings which may be present in
       :  * a SIP Diversion header
       :  */
       : static const struct reasons {
       : 	enum AST_REDIRECTING_REASON code;
       : 	const char *text;
       : } reason_table[] = {
       : 	{ AST_REDIRECTING_REASON_UNKNOWN, "unknown" },
       : 	{ AST_REDIRECTING_REASON_USER_BUSY, "user-busy" },
       : 	{ AST_REDIRECTING_REASON_NO_ANSWER, "no-answer" },
       : 	{ AST_REDIRECTING_REASON_UNAVAILABLE, "unavailable" },
       : 	{ AST_REDIRECTING_REASON_UNCONDITIONAL, "unconditional" },
       : 	{ AST_REDIRECTING_REASON_TIME_OF_DAY, "time-of-day" },
       : 	{ AST_REDIRECTING_REASON_DO_NOT_DISTURB, "do-not-disturb" },
       : 	{ AST_REDIRECTING_REASON_DEFLECTION, "deflection" },
       : 	{ AST_REDIRECTING_REASON_FOLLOW_ME, "follow-me" },
       : 	{ AST_REDIRECTING_REASON_OUT_OF_ORDER, "out-of-service" },
       : 	{ AST_REDIRECTING_REASON_AWAY, "away" },
       : 	{ AST_REDIRECTING_REASON_CALL_FWD_DTE, "cf_dte" },		/* Non-standard */
       : 	{ AST_REDIRECTING_REASON_SEND_TO_VM, "send_to_vm" },	/* Non-standard */
       : };
       : 
       : static const char *reason_code_to_str(const struct ast_party_redirecting_reason *reason)
       : {
       : 	int idx;
       : 	int code;
       : 
       : 	/* use specific string if given */
       : 	if (!ast_strlen_zero(reason->str)) {
       : 		return reason->str;
       : 	}
       : 
       : 	code = reason->code;
       : 	for (idx = 0; idx < ARRAY_LEN(reason_table); ++idx) {
       : 		if (code == reason_table[idx].code) {
       : 			return reason_table[idx].text;
       : 		}
       : 	}
       : 
       : 	return "unknown";
       : }
> All this code looks to be duplicated between chan_sip and here. Since these
I'm not sure moving sip_is_token() and reason_code_to_str() should be moved into Asterisk's core (main/callerid.c) since they would only be useful to SIP channel drivers.  Since we have two SIP channel drivers, I suppose we could create a common SIP utility resource module.  However, that would be a fairly large patch in and of itself and cause chan_sip and res_pjsip to be dependent on another common module.

What say Mark and/or Josh about it?

The ast_redirecting_reason_name_function() absolutely cannot be used in place of reason_str_to_code().  That function returns the original ISDN name strings and not the corresponding SIP name strings that are required.  I made chan_sip and pjsip use the ast_redirecting_reason_parse() function because I explicitly added the SIP aliases to the conversion table rather than keep the duplicated reason_str_to_code().  The added aliases also have a benefit to the user if they set the REDIRECTING(reason) string.


-- 
To view, visit https://gerrit.asterisk.org/2331
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifba83d23a195a9f64d55b9c681d2e62476b68a87
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes