[Asterisk-code-review] func_sayfiles: Retrieve say file names (asterisk[master])
N A
asteriskteam at digium.com
Thu Aug 19 15:15:04 CDT 2021
Attention is currently required from: Joshua Colp, George Joseph, Kevin Harwell.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16226 )
Change subject: func_sayfiles: Retrieve say file names
......................................................................
Patch Set 13:
(30 comments)
File funcs/func_sayfiles.c:
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/bcc296b5_abf416d6
PS12, Line 51: <parameter name="value">
> s/value/type
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/c9e976a6_149c21c2
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 […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/ee31948b_50600697
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. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/809eb01c_0e61be11
PS12, Line 129: if (sscanf(value, "%d", &num) == 1)
> Use ast_str_to_int instead (found in conversions. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/6cdcd85d_d5029b7f
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.
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/160e2812_c0657fba
PS12, Line 135: if (sscanf(value, "%d", &num) == 1)
> Use ast_str_to_int instead (found in conversions. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/36e5fe0b_9ab4c51d
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.
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/6e4c42b9_e8b83f75
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¢s") != 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¢") != 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¢s") != 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¢s") != 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¢s") != 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 […]
I thought it would be more helpful to see which ones are failing/succeeding if something goes wrong, but I don't really have a good answer for this. I just copied the structure that func_math uses for testing.
File main/pbx_builtins.c:
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/5eec6054_464972cc
PS12, Line 528: in the current language. Currently only English and US Dollars is supported.
> Remove extra whitespace at end of line.
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/3986a13c_21be8aab
PS12, Line 1396: if (sscanf(tmp, "%d", &number_val) != 1) {
> Use ast_str_to_int here.
Done
File main/say.c:
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/942ebd89_f1e1b7f6
PS12, Line 72: struct ast_str *filenames = ast_str_create(20);
> Check for NULL return
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/4cc9cf4e_3ad9b4da
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). […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/8d47692f_1ca007e1
PS12, Line 202: struct ast_str *filenames = ast_str_create(20);
: ast_str_reset(filenames);
> Check for NULL return
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/b171f9b9_d2f158dc
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
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/b546c153_557c9b07
PS12, Line 300: struct ast_str *filenames = ast_str_create(20);
: ast_str_reset(filenames);
> Check for NULL return
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/fb5c2a2e_e444359e
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
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/09ed49b4_7856a60b
PS12, Line 371: struct ast_str *filenames = ast_str_create(20);
> Check for NULL return
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/14f463ba_517c95a8
PS12, Line 385: fnrecurse = ast_get_number_str((amt / 100), lang);
> Check for NULL return
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/7c07adbb_b53a6e96
PS12, Line 399: }
> fnrecurse is leaked and needs to be freed here
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/c6598ce7_5c817874
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
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/5049de1c_f45a6ad9
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
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/e42d37aa_04b8bab6
PS12, Line 465: if (!num)
: return ast_get_digit_str("0", lang);
> Add braces
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/04ef7c35_7a68b1b8
PS12, Line 468: filenames = ast_str_create(20);
> check for NULL
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/bba1c1a6_37808662
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. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/965293e9_18a4d62a
PS12, Line 557: if (!num)
: num = 0;
> Add braces
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/4da861b9_bad794cb
PS12, Line 560: filenames = ast_str_create(20);
> Needs NULL check
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/7ef3ea38_17125fa6
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.
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/719979ff_c25896af
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. […]
I tried doing this but ran into crashes and core dumps with memory issues that didn't really make sense, here is what I tried for that: https://paste.interlinked.us/1tw6wjlpmt.txt
Just reverting this component makes everything else work.
Some of the logic is similar but enough of it is different that it gets a little bit messy, as the snippet shows.
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/3616d27c_296621cf
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
Done
https://gerrit.asterisk.org/c/asterisk/+/16226/comment/fc53911b_146965b6
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
Done
--
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: 13
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: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 19 Aug 2021 20:15:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210819/7ca44f5c/attachment-0001.html>
More information about the asterisk-code-review
mailing list