[Asterisk-code-review] func_sayfiles: Retrieve say file names (asterisk[master])

Kevin Harwell asteriskteam at digium.com
Thu Aug 19 13:28:36 CDT 2021


Attention is currently required from: N A, Joshua Colp, George Joseph.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16226 )

Change subject: func_sayfiles: Retrieve say file names
......................................................................


Patch Set 12: Code-Review-1

(30 comments)

File funcs/func_sayfiles.c:

https://gerrit.asterisk.org/c/asterisk/+/16226/comment/668e8f70_11da80af 
PS12, Line 51: 			<parameter name="value">
s/value/type


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/b4c38164_195aa28a 
PS12, Line 76: 			<para>Returns the files that would be played by a Say application. These filenames could then be
             : 			passed directly into Playback, BackGround, Read, Queue, or any application which supports
             : 			playback of multiple ampersand-delimited files.</para>
I know it can be implied by the text here, but I think it'd be better to explicitly say "Returns an ampersand delimited string of files that ...."


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/2706996d_d838fb5e 
PS12, Line 111: 	if (ast_strlen_zero(args.type))
              : 		type = "alpha";
              : 	else
              : 		type = args.type;
              : 
              : 	if (chan)
              : 		lang = ast_channel_language(chan);
              : 	else
              : 		lang = "en"; /* No chan for unit tests */
Per coding guidelines if/else need to have braces even if single line.

Alternatively you could use the ternary ( condition ? true : false ) operator.


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/50b79b85_9420ffbd 
PS12, Line 129: if (sscanf(value, "%d", &num) == 1)
Use ast_str_to_int instead (found in conversions.h)


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/15198572_f8c0cacd 
PS12, Line 129: 		if (sscanf(value, "%d", &num) == 1)
              : 			filenames = ast_get_number_str(num, lang);
              : 		else
              : 			ast_log(LOG_WARNING, "Invalid numeric argument: %s\n", value);
Add braces to if/else.


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/aafc2106_996f65d8 
PS12, Line 135: if (sscanf(value, "%d", &num) == 1)
Use ast_str_to_int instead (found in conversions.h)


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/fcc5661f_6dec9c85 
PS12, Line 134: int num;
              : 		if (sscanf(value, "%d", &num) == 1)
              : 			filenames = ast_get_ordinal_str(num, lang);
              : 		else
              : 			ast_log(LOG_WARNING, "Invalid numeric argument: %s\n", value);
Add braces to if/else.


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/e75e192c_0ccde3b5 
PS12, Line 165: 	struct ast_str *expr, *result;
              : 
              : 	switch (cmd) {
              : 	case TEST_INIT:
              : 		info->name = "test_SAYFILES_function";
              : 		info->category = "/funcs/func_sayfiles/";
              : 		info->summary = "Test SAYFILES function substitution";
              : 		info->description =
              : 			"Executes a series of variable substitutions using the SAYFILES function and ensures that the expected results are received.";
              : 		return AST_TEST_NOT_RUN;
              : 	case TEST_EXECUTE:
              : 		break;
              : 	}
              : 
              : 	ast_test_status_update(test, "Testing SAYFILES() substitution ...\n");
              : 
              : 	if (!(expr = ast_str_create(16))) {
              : 		return AST_TEST_FAIL;
              : 	}
              : 	if (!(result = ast_str_create(16))) {
              : 		ast_free(expr);
              : 		return AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(hi Th3re,alpha)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "letters/h&letters/i&letters/space&letters/t&letters/h&digits/3&letters/r&letters/e") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(hi Th3re,alpha) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(phreak,phonetic)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "phonetic/p_p&phonetic/h_p&phonetic/r_p&phonetic/e_p&phonetic/a_p&phonetic/k_p") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(phreak,phonetic) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(35,digits)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/3&digits/5") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(35,digits) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(35,number)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/30&digits/5") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(35,number) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(1042,number)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/1&digits/thousand&digits/40&digits/2") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(1042,number) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(0,number)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/0") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(0,digits) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(2001000001,number)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/2&digits/billion&digits/1&digits/million&digits/1") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(2001000001,number) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(7,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/h-7") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(7,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(35,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/30&digits/h-5") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(35,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(1042,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/1&digits/thousand&digits/40&digits/h-2") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(1042,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(11042,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/11&digits/thousand&digits/40&digits/h-2") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(11042,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(40000,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/40&digits/h-thousand") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(40000,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(43638,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/40&digits/3&digits/thousand&digits/6&digits/hundred&digits/30&digits/h-8") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(43638,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(1000000,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/1&digits/h-million") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(1000000,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(1000001,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/1&digits/million&digits/h-1") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(1000001,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(2001000001,ordinal)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/2&digits/billion&digits/1&digits/million&digits/h-1") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(2001000001,ordinal) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(0,money)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/0&cents") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(0,money) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(0.01,money)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/1&cent") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(0.01,money) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(0.42,money)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/40&digits/2&cents") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(0.42,money) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(1.00,money)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/1&letters/dollar") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(1.00,money) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(1.42,money)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/1&letters/dollar_&and&digits/40&digits/2&cents") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(1.42,money) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(2.00,money)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/2&dollars") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(2.00,money) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_str_set(&expr, 0, "${SAYFILES(2.42,money)}");
              : 	ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));
              : 	if (strcmp(ast_str_buffer(result), "digits/2&dollars&and&digits/40&digits/2&cents") != 0) {
              : 		ast_test_status_update(test, "SAYFILES(2.42,money) test failed ('%s')\n",
              : 				ast_str_buffer(result));
              : 		res = AST_TEST_FAIL;
              : 	}
              : 
              : 	ast_free(expr);
              : 	ast_free(result);
              : 
              : 	return res;
Any reason to not just return early on failure? You could convert 'expr' and 'result' to RAII_VAR's and simply return in each path. Alternatively you could use a 'goto' to jump to the failure case.


File main/pbx_builtins.c:

https://gerrit.asterisk.org/c/asterisk/+/16226/comment/31670995_9d36585b 
PS12, Line 528: 			in the current language. Currently only English and US Dollars is supported. 
Remove extra whitespace at end of line.


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/98f7c9f9_f1dc5006 
PS12, Line 1396: if (sscanf(tmp, "%d", &number_val) != 1) {
Use ast_str_to_int here.


File main/say.c:

https://gerrit.asterisk.org/c/asterisk/+/16226/comment/4a5fefd5_691091db 
PS12, Line 72: 	struct ast_str *filenames = ast_str_create(20);
Check for NULL return


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/c86ece0e_786212ad 
PS12, Line 177: 	char *files = ast_str_buffer(filenames);
              : 
              : 	while ((fn = strsep(&files, "&"))) {
              : 		res = ast_streamfile(chan, fn, lang);
              : 		if (!res) {
              : 			if ((audiofd  > -1) && (ctrlfd > -1))
              : 				res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);
              : 			else
              : 				res = ast_waitstream(chan, ints);
              : 		}
              : 		ast_stopstream(chan);
              : 	}
              : 
              : 	ast_free(filenames);
This code appears to be duplicated in several places (1/6). Move it into its own function that can then be called from each use case.

Also be sure to check that filesnames is not NULL.


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/cbcdb6ff_bc1da397 
PS12, Line 202: 	struct ast_str *filenames = ast_str_create(20);
              : 	ast_str_reset(filenames);
Check for NULL return


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/7c4419b9_342011b9 
PS12, Line 276: 	char *files = ast_str_buffer(filenames);
              : 
              : 	while ((fn = strsep(&files, "&"))) {
              : 		res = ast_streamfile(chan, fn, lang);
              : 		if (!res) {
              : 			if ((audiofd  > -1) && (ctrlfd > -1))
              : 				res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);
              : 			else
              : 				res = ast_waitstream(chan, ints);
              : 		}
              : 		ast_stopstream(chan);
              : 	}
              : 
              : 	ast_free(filenames);
Duped code (2/6), combine into a single function


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/ce7d45d1_79275fa8 
PS12, Line 300: 	struct ast_str *filenames = ast_str_create(20);
              : 	ast_str_reset(filenames);
Check for NULL return


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/6e93e882_c17fc68b 
PS12, Line 345: 	char *files = ast_str_buffer(filenames);
              : 
              : 	while ((fn = strsep(&files, "&"))) {
              : 		res = ast_streamfile(chan, fn, lang);
              : 		if (!res) {
              : 			if ((audiofd  > -1) && (ctrlfd > -1))
              : 				res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);
              : 			else
              : 				res = ast_waitstream(chan, ints);
              : 		}
              : 		ast_stopstream(chan);
              : 	}
              : 
              : 	ast_free(filenames);
Duped code (3/6), combine into a single function


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/79dd55c6_661e9cbc 
PS12, Line 371: 	struct ast_str *filenames = ast_str_create(20);
Check for NULL return


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/a43699ff_00775c26 
PS12, Line 385: fnrecurse = ast_get_number_str((amt / 100), lang);
Check for NULL return


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/d8a12131_5a1e6916 
PS12, Line 399: 		}
fnrecurse is leaked and needs to be freed here


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/f24895f3_e83b1ac5 
PS12, Line 402: 	if (cents > 0) {
              : 		ast_debug(1, "Entered cents block\n");
              : 		fnrecurse = ast_get_number_str(cents, lang);
              : 		fnr = ast_str_buffer(fnrecurse);
              : 		ast_str_append(&filenames, 0, (amt < 100 ? "%s" : "&%s"), fnr);
              : 		ast_str_append(&filenames, 0, "&%s", (cents == 1) ? "cent" : "cents");
              : 	} else if (amt == 0) {
              : 		fnrecurse = ast_get_digit_str("0", lang);
              : 		fnr = ast_str_buffer(fnrecurse);
              : 		ast_str_append(&filenames, 0, "%s", fnr);
              : 		ast_str_append(&filenames, 0, "&%s", "cents");
              : 	}
fnrecurse is leaked in both of the paths and needs to be freed


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/bbf43826_3d104d81 
PS12, Line 436: 	char *files = ast_str_buffer(filenames);
              : 
              : 	while ((fn = strsep(&files, "&"))) {
              : 		res = ast_streamfile(chan, fn, lang);
              : 		if (!res) {
              : 			if ((audiofd  > -1) && (ctrlfd > -1))
              : 				res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);
              : 			else
              : 				res = ast_waitstream(chan, ints);
              : 		}
              : 		ast_stopstream(chan);
              : 	}
              : 
              : 	ast_free(filenames);
Duped code (4/6), combine into a single function


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/34c95914_ad97815a 
PS12, Line 465: 	if (!num)
              : 		return ast_get_digit_str("0", lang);
Add braces


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/52fa6088_4c2c9738 
PS12, Line 468: filenames = ast_str_create(20);
check for NULL


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/687b4a2c_79c7c143 
PS12, Line 496: fnrecurse = get_number_str_en((num / 1000), lang);
              : 					fnr = ast_str_buffer(fnrecurse);
              : 					ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 					num %= 1000;
              : 					snprintf(fn, sizeof(fn), "&digits/thousand");
              : 				} else {
              : 					if (num < 1000000000) {	/* 1,000,000,000 */
              : 						fnrecurse = get_number_str_en((num / 1000000), lang);
              : 						fnr = ast_str_buffer(fnrecurse);
              : 						ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 						num %= 1000000;
              : 						ast_copy_string(fn, "&digits/million", sizeof(fn));
              : 					} else {
              : 						if (num < INT_MAX) {
              : 							fnrecurse = get_number_str_en((num / 1000000000), lang);
              : 							fnr = ast_str_buffer(fnrecurse);
              : 							ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 							num %= 1000000000;
              : 							ast_copy_string(fn, "&digits/billion", sizeof(fn));
              : 						} else {
              : 							ast_log(LOG_WARNING, "Number '%d' is too big for me\n", num);
              : 							res = -1;
              : 						}
              : 					}
When fnrecurse is obtained it needs to be checked for NULL. Also it's being leaked in each used path.


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/856c9ce3_10152ba0 
PS12, Line 557: 	if (!num)
              : 		num = 0;
Add braces


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/ae639b99_e39e5aee 
PS12, Line 560: 	filenames = ast_str_create(20);
Needs NULL check


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/bb502edc_91c02c49 
PS12, Line 597: 					fnrecurse = get_number_str_en((num / 1000), lang);
              : 					fnr = ast_str_buffer(fnrecurse);
              : 					ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 					num %= 1000;
              : 					snprintf(fn, sizeof(fn), (num % 1000 == 0) ? "&digits/h-thousand" : "&digits/thousand");
              : 				} else {
              : 					if (num < 1000000000) {	/* 1,000,000,000 */
              : 						fnrecurse = get_number_str_en((num / 1000000), lang);
              : 						fnr = ast_str_buffer(fnrecurse);
              : 						ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 						num %= 1000000;
              : 						ast_copy_string(fn, (num % 1000000 == 0) ? "&digits/h-million" : "&digits/million", sizeof(fn));
              : 					} else {
              : 						if (num < INT_MAX) {
              : 							fnrecurse = get_number_str_en((num / 1000000000), lang);
              : 							fnr = ast_str_buffer(fnrecurse);
              : 							ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 							num %= 1000000000;
              : 							ast_copy_string(fn, (num % 1000000000 == 0) ? "&digits/h-billion" : "&digits/billion", 
fnrecurse needs NULL checks when retrieved, and needs to be freed when no longer needed.


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/7120f82e_2f26aad1 
PS12, Line 563: 	while (!res && (num || playh)) {
              : 		if (num < 0) {
              : 			ast_copy_string(fn, "digits/minus", sizeof(fn));
              : 			if ( num > INT_MIN ) {
              : 				num = -num;
              : 			} else {
              : 				num = 0;
              : 			}
              : 		} else if (playh) {
              : 			ast_copy_string(fn, (num % 100 == 0) ? "digits/h-hundred" : "digits/hundred", sizeof(fn));
              : 			playh = 0;
              : 		} else if (num < 20) {
              : 			if (num > 0) {
              : 				snprintf(fn, sizeof(fn), "digits/h-%d", num);
              : 			} else {
              : 				ast_log(LOG_ERROR, "Unsupported ordinal number: %d\n", num);
              : 			}
              : 			num = 0;
              : 		} else if (num < 100) {
              : 			int base = (num / 10) * 10;
              : 			if (base != num) {
              : 				snprintf(fn, sizeof(fn), "digits/%d", base);
              : 			} else {
              : 				snprintf(fn, sizeof(fn), "digits/h-%d", base);
              : 			}
              : 			num %= 10;
              : 		} else {
              : 			if (num < 1000){
              : 				snprintf(fn, sizeof(fn), "digits/%d", (num/100));
              : 				playh++;
              : 				num %= 100;
              : 			} else {
              : 				struct ast_str *fnrecurse;
              : 				if (num < 1000000) { /* 1,000,000 */
              : 					fnrecurse = get_number_str_en((num / 1000), lang);
              : 					fnr = ast_str_buffer(fnrecurse);
              : 					ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 					num %= 1000;
              : 					snprintf(fn, sizeof(fn), (num % 1000 == 0) ? "&digits/h-thousand" : "&digits/thousand");
              : 				} else {
              : 					if (num < 1000000000) {	/* 1,000,000,000 */
              : 						fnrecurse = get_number_str_en((num / 1000000), lang);
              : 						fnr = ast_str_buffer(fnrecurse);
              : 						ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 						num %= 1000000;
              : 						ast_copy_string(fn, (num % 1000000 == 0) ? "&digits/h-million" : "&digits/million", sizeof(fn));
              : 					} else {
              : 						if (num < INT_MAX) {
              : 							fnrecurse = get_number_str_en((num / 1000000000), lang);
              : 							fnr = ast_str_buffer(fnrecurse);
              : 							ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);
              : 							num %= 1000000000;
              : 							ast_copy_string(fn, (num % 1000000000 == 0) ? "&digits/h-billion" : "&digits/billion", sizeof(fn));
              : 						} else {
              : 							ast_log(LOG_WARNING, "Number '%d' is too big for me\n", num);
              : 							res = -1;
              : 						}
              : 					}
              : 				}
              : 				/* we already decided whether or not to add an &, don't add another one immediately */
              : 				loops = 0;
              : 			}
              : 		}
              : 		if (!res) {
              : 			ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fn);
              : 			loops++;
              : 		}
A lot of this code looks duped or very similar to what's in the above 'get_number_str_en' function. Think there is anyway to combine into a shared function for some or all?


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/89fe5342_668fb735 
PS12, Line 889: 	char *files = ast_str_buffer(filenames);
              : 
              : 	while ((fn = strsep(&files, "&"))) {
              : 		res = ast_streamfile(chan, fn, language);
              : 		if (!res) {
              : 			if ((audiofd  > -1) && (ctrlfd > -1))
              : 				res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);
              : 			else
              : 				res = ast_waitstream(chan, ints);
              : 		}
              : 		ast_stopstream(chan);
              : 	}
              : 
              : 	ast_free(filenames);
Duped code (5/6), combine into a single function


https://gerrit.asterisk.org/c/asterisk/+/16226/comment/d8028e9d_5d96214f 
PS12, Line 914: 	char *files = ast_str_buffer(filenames);
              : 
              : 	while ((fn = strsep(&files, "&"))) {
              : 		res = ast_streamfile(chan, fn, language);
              : 		if (!res) {
              : 			if ((audiofd  > -1) && (ctrlfd > -1))
              : 				res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);
              : 			else
              : 				res = ast_waitstream(chan, ints);
              : 		}
              : 		ast_stopstream(chan);
              : 	}
              : 
              : 	ast_free(filenames);
Duped code (6/6), combine into a single function



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16226
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: If9718c89353b8e153d84add3cc4637b79585db19
Gerrit-Change-Number: 16226
Gerrit-PatchSet: 12
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Thu, 19 Aug 2021 18:28:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210819/c6cf4f44/attachment-0001.html>


More information about the asterisk-code-review mailing list