<p> Attention is currently required from: Joshua Colp, George Joseph, Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/16226">View Change</a></p><p>30 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File funcs/func_sayfiles.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/bcc296b5_abf416d6">Patch Set #12, Line 51:</a> <code style="font-family:monospace,monospace"> <parameter name="value"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/value/type</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/c9e976a6_149c21c2">Patch Set #12, Line 76:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> <para>Returns the files that would be played by a Say application. These filenames could then be<br> passed directly into Playback, BackGround, Read, Queue, or any application which supports<br> playback of multiple ampersand-delimited files.</para><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I know it can be implied by the text here, but I think it'd be better to explicitly say "Returns an […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/ee31948b_50600697">Patch Set #12, Line 111:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (ast_strlen_zero(args.type))<br> type = "alpha";<br> else<br> type = args.type;<br><br> if (chan)<br> lang = ast_channel_language(chan);<br> else<br> lang = "en"; /* No chan for unit tests */<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Per coding guidelines if/else need to have braces even if single line. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/809eb01c_0e61be11">Patch Set #12, Line 129:</a> <code style="font-family:monospace,monospace">if (sscanf(value, "%d", &num) == 1)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Use ast_str_to_int instead (found in conversions. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/6cdcd85d_d5029b7f">Patch Set #12, Line 129:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (sscanf(value, "%d", &num) == 1)<br> filenames = ast_get_number_str(num, lang);<br> else<br> ast_log(LOG_WARNING, "Invalid numeric argument: %s\n", value);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add braces to if/else.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/160e2812_c0657fba">Patch Set #12, Line 135:</a> <code style="font-family:monospace,monospace">if (sscanf(value, "%d", &num) == 1)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Use ast_str_to_int instead (found in conversions. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/36e5fe0b_9ab4c51d">Patch Set #12, Line 134:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">int num;<br> if (sscanf(value, "%d", &num) == 1)<br> filenames = ast_get_ordinal_str(num, lang);<br> else<br> ast_log(LOG_WARNING, "Invalid numeric argument: %s\n", value);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add braces to if/else.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/6e4c42b9_e8b83f75">Patch Set #12, Line 165:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct ast_str *expr, *result;<br><br> switch (cmd) {<br> case TEST_INIT:<br> info->name = "test_SAYFILES_function";<br> info->category = "/funcs/func_sayfiles/";<br> info->summary = "Test SAYFILES function substitution";<br> info->description =<br> "Executes a series of variable substitutions using the SAYFILES function and ensures that the expected results are received.";<br> return AST_TEST_NOT_RUN;<br> case TEST_EXECUTE:<br> break;<br> }<br><br> ast_test_status_update(test, "Testing SAYFILES() substitution ...\n");<br><br> if (!(expr = ast_str_create(16))) {<br> return AST_TEST_FAIL;<br> }<br> if (!(result = ast_str_create(16))) {<br> ast_free(expr);<br> return AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(hi Th3re,alpha)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "letters/h&letters/i&letters/space&letters/t&letters/h&digits/3&letters/r&letters/e") != 0) {<br> ast_test_status_update(test, "SAYFILES(hi Th3re,alpha) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(phreak,phonetic)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> 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) {<br> ast_test_status_update(test, "SAYFILES(phreak,phonetic) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(35,digits)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/3&digits/5") != 0) {<br> ast_test_status_update(test, "SAYFILES(35,digits) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(35,number)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/30&digits/5") != 0) {<br> ast_test_status_update(test, "SAYFILES(35,number) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(1042,number)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/1&digits/thousand&digits/40&digits/2") != 0) {<br> ast_test_status_update(test, "SAYFILES(1042,number) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(0,number)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/0") != 0) {<br> ast_test_status_update(test, "SAYFILES(0,digits) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(2001000001,number)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/2&digits/billion&digits/1&digits/million&digits/1") != 0) {<br> ast_test_status_update(test, "SAYFILES(2001000001,number) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(7,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/h-7") != 0) {<br> ast_test_status_update(test, "SAYFILES(7,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(35,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/30&digits/h-5") != 0) {<br> ast_test_status_update(test, "SAYFILES(35,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(1042,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/1&digits/thousand&digits/40&digits/h-2") != 0) {<br> ast_test_status_update(test, "SAYFILES(1042,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(11042,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/11&digits/thousand&digits/40&digits/h-2") != 0) {<br> ast_test_status_update(test, "SAYFILES(11042,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(40000,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/40&digits/h-thousand") != 0) {<br> ast_test_status_update(test, "SAYFILES(40000,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(43638,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/40&digits/3&digits/thousand&digits/6&digits/hundred&digits/30&digits/h-8") != 0) {<br> ast_test_status_update(test, "SAYFILES(43638,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(1000000,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/1&digits/h-million") != 0) {<br> ast_test_status_update(test, "SAYFILES(1000000,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(1000001,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/1&digits/million&digits/h-1") != 0) {<br> ast_test_status_update(test, "SAYFILES(1000001,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(2001000001,ordinal)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/2&digits/billion&digits/1&digits/million&digits/h-1") != 0) {<br> ast_test_status_update(test, "SAYFILES(2001000001,ordinal) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(0,money)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/0¢s") != 0) {<br> ast_test_status_update(test, "SAYFILES(0,money) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(0.01,money)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/1¢") != 0) {<br> ast_test_status_update(test, "SAYFILES(0.01,money) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(0.42,money)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/40&digits/2¢s") != 0) {<br> ast_test_status_update(test, "SAYFILES(0.42,money) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(1.00,money)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/1&letters/dollar") != 0) {<br> ast_test_status_update(test, "SAYFILES(1.00,money) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(1.42,money)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/1&letters/dollar_&and&digits/40&digits/2¢s") != 0) {<br> ast_test_status_update(test, "SAYFILES(1.42,money) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(2.00,money)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/2&dollars") != 0) {<br> ast_test_status_update(test, "SAYFILES(2.00,money) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_str_set(&expr, 0, "${SAYFILES(2.42,money)}");<br> ast_str_substitute_variables(&result, 0, NULL, ast_str_buffer(expr));<br> if (strcmp(ast_str_buffer(result), "digits/2&dollars&and&digits/40&digits/2¢s") != 0) {<br> ast_test_status_update(test, "SAYFILES(2.42,money) test failed ('%s')\n",<br> ast_str_buffer(result));<br> res = AST_TEST_FAIL;<br> }<br><br> ast_free(expr);<br> ast_free(result);<br><br> return res;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Any reason to not just return early on failure? You could convert 'expr' and 'result' to RAII_VAR's […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File main/pbx_builtins.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/5eec6054_464972cc">Patch Set #12, Line 528:</a> <code style="font-family:monospace,monospace"> in the current language. Currently only English and US Dollars is supported. </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Remove extra whitespace at end of line.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/3986a13c_21be8aab">Patch Set #12, Line 1396:</a> <code style="font-family:monospace,monospace">if (sscanf(tmp, "%d", &number_val) != 1) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Use ast_str_to_int here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File main/say.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/942ebd89_f1e1b7f6">Patch Set #12, Line 72:</a> <code style="font-family:monospace,monospace"> struct ast_str *filenames = ast_str_create(20);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Check for NULL return</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/4cc9cf4e_3ad9b4da">Patch Set #12, Line 177:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> char *files = ast_str_buffer(filenames);<br><br> while ((fn = strsep(&files, "&"))) {<br> res = ast_streamfile(chan, fn, lang);<br> if (!res) {<br> if ((audiofd > -1) && (ctrlfd > -1))<br> res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);<br> else<br> res = ast_waitstream(chan, ints);<br> }<br> ast_stopstream(chan);<br> }<br><br> ast_free(filenames);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This code appears to be duplicated in several places (1/6). […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/8d47692f_1ca007e1">Patch Set #12, Line 202:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct ast_str *filenames = ast_str_create(20);<br> ast_str_reset(filenames);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Check for NULL return</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/b171f9b9_d2f158dc">Patch Set #12, Line 276:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> char *files = ast_str_buffer(filenames);<br><br> while ((fn = strsep(&files, "&"))) {<br> res = ast_streamfile(chan, fn, lang);<br> if (!res) {<br> if ((audiofd > -1) && (ctrlfd > -1))<br> res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);<br> else<br> res = ast_waitstream(chan, ints);<br> }<br> ast_stopstream(chan);<br> }<br><br> ast_free(filenames);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Duped code (2/6), combine into a single function</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/b546c153_557c9b07">Patch Set #12, Line 300:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> struct ast_str *filenames = ast_str_create(20);<br> ast_str_reset(filenames);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Check for NULL return</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/fb5c2a2e_e444359e">Patch Set #12, Line 345:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> char *files = ast_str_buffer(filenames);<br><br> while ((fn = strsep(&files, "&"))) {<br> res = ast_streamfile(chan, fn, lang);<br> if (!res) {<br> if ((audiofd > -1) && (ctrlfd > -1))<br> res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);<br> else<br> res = ast_waitstream(chan, ints);<br> }<br> ast_stopstream(chan);<br> }<br><br> ast_free(filenames);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Duped code (3/6), combine into a single function</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/09ed49b4_7856a60b">Patch Set #12, Line 371:</a> <code style="font-family:monospace,monospace"> struct ast_str *filenames = ast_str_create(20);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Check for NULL return</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/14f463ba_517c95a8">Patch Set #12, Line 385:</a> <code style="font-family:monospace,monospace">fnrecurse = ast_get_number_str((amt / 100), lang);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Check for NULL return</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/7c07adbb_b53a6e96">Patch Set #12, Line 399:</a> <code style="font-family:monospace,monospace"> }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">fnrecurse is leaked and needs to be freed here</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/c6598ce7_5c817874">Patch Set #12, Line 402:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (cents > 0) {<br> ast_debug(1, "Entered cents block\n");<br> fnrecurse = ast_get_number_str(cents, lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (amt < 100 ? "%s" : "&%s"), fnr);<br> ast_str_append(&filenames, 0, "&%s", (cents == 1) ? "cent" : "cents");<br> } else if (amt == 0) {<br> fnrecurse = ast_get_digit_str("0", lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, "%s", fnr);<br> ast_str_append(&filenames, 0, "&%s", "cents");<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">fnrecurse is leaked in both of the paths and needs to be freed</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/5049de1c_f45a6ad9">Patch Set #12, Line 436:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> char *files = ast_str_buffer(filenames);<br><br> while ((fn = strsep(&files, "&"))) {<br> res = ast_streamfile(chan, fn, lang);<br> if (!res) {<br> if ((audiofd > -1) && (ctrlfd > -1))<br> res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);<br> else<br> res = ast_waitstream(chan, ints);<br> }<br> ast_stopstream(chan);<br> }<br><br> ast_free(filenames);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Duped code (4/6), combine into a single function</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/e42d37aa_04b8bab6">Patch Set #12, Line 465:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (!num)<br> return ast_get_digit_str("0", lang);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add braces</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/04ef7c35_7a68b1b8">Patch Set #12, Line 468:</a> <code style="font-family:monospace,monospace">filenames = ast_str_create(20);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">check for NULL</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/bba1c1a6_37808662">Patch Set #12, Line 496:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">fnrecurse = get_number_str_en((num / 1000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000;<br> snprintf(fn, sizeof(fn), "&digits/thousand");<br> } else {<br> if (num < 1000000000) { /* 1,000,000,000 */<br> fnrecurse = get_number_str_en((num / 1000000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000000;<br> ast_copy_string(fn, "&digits/million", sizeof(fn));<br> } else {<br> if (num < INT_MAX) {<br> fnrecurse = get_number_str_en((num / 1000000000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000000000;<br> ast_copy_string(fn, "&digits/billion", sizeof(fn));<br> } else {<br> ast_log(LOG_WARNING, "Number '%d' is too big for me\n", num);<br> res = -1;<br> }<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">When fnrecurse is obtained it needs to be checked for NULL. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/965293e9_18a4d62a">Patch Set #12, Line 557:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (!num)<br> num = 0;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add braces</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/4da861b9_bad794cb">Patch Set #12, Line 560:</a> <code style="font-family:monospace,monospace"> filenames = ast_str_create(20);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Needs NULL check</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/7ef3ea38_17125fa6">Patch Set #12, Line 597:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> fnrecurse = get_number_str_en((num / 1000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000;<br> snprintf(fn, sizeof(fn), (num % 1000 == 0) ? "&digits/h-thousand" : "&digits/thousand");<br> } else {<br> if (num < 1000000000) { /* 1,000,000,000 */<br> fnrecurse = get_number_str_en((num / 1000000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000000;<br> ast_copy_string(fn, (num % 1000000 == 0) ? "&digits/h-million" : "&digits/million", sizeof(fn));<br> } else {<br> if (num < INT_MAX) {<br> fnrecurse = get_number_str_en((num / 1000000000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000000000;<br> ast_copy_string(fn, (num % 1000000000 == 0) ? "&digits/h-billion" : "&digits/billion", <br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">fnrecurse needs NULL checks when retrieved, and needs to be freed when no longer needed.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/719979ff_c25896af">Patch Set #12, Line 563:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> while (!res && (num || playh)) {<br> if (num < 0) {<br> ast_copy_string(fn, "digits/minus", sizeof(fn));<br> if ( num > INT_MIN ) {<br> num = -num;<br> } else {<br> num = 0;<br> }<br> } else if (playh) {<br> ast_copy_string(fn, (num % 100 == 0) ? "digits/h-hundred" : "digits/hundred", sizeof(fn));<br> playh = 0;<br> } else if (num < 20) {<br> if (num > 0) {<br> snprintf(fn, sizeof(fn), "digits/h-%d", num);<br> } else {<br> ast_log(LOG_ERROR, "Unsupported ordinal number: %d\n", num);<br> }<br> num = 0;<br> } else if (num < 100) {<br> int base = (num / 10) * 10;<br> if (base != num) {<br> snprintf(fn, sizeof(fn), "digits/%d", base);<br> } else {<br> snprintf(fn, sizeof(fn), "digits/h-%d", base);<br> }<br> num %= 10;<br> } else {<br> if (num < 1000){<br> snprintf(fn, sizeof(fn), "digits/%d", (num/100));<br> playh++;<br> num %= 100;<br> } else {<br> struct ast_str *fnrecurse;<br> if (num < 1000000) { /* 1,000,000 */<br> fnrecurse = get_number_str_en((num / 1000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000;<br> snprintf(fn, sizeof(fn), (num % 1000 == 0) ? "&digits/h-thousand" : "&digits/thousand");<br> } else {<br> if (num < 1000000000) { /* 1,000,000,000 */<br> fnrecurse = get_number_str_en((num / 1000000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000000;<br> ast_copy_string(fn, (num % 1000000 == 0) ? "&digits/h-million" : "&digits/million", sizeof(fn));<br> } else {<br> if (num < INT_MAX) {<br> fnrecurse = get_number_str_en((num / 1000000000), lang);<br> fnr = ast_str_buffer(fnrecurse);<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fnr);<br> num %= 1000000000;<br> ast_copy_string(fn, (num % 1000000000 == 0) ? "&digits/h-billion" : "&digits/billion", sizeof(fn));<br> } else {<br> ast_log(LOG_WARNING, "Number '%d' is too big for me\n", num);<br> res = -1;<br> }<br> }<br> }<br> /* we already decided whether or not to add an &, don't add another one immediately */<br> loops = 0;<br> }<br> }<br> if (!res) {<br> ast_str_append(&filenames, 0, (loops == 0 ? "%s" : "&%s"), fn);<br> loops++;<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">A lot of this code looks duped or very similar to what's in the above 'get_number_str_en' function. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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<br>Just reverting this component makes everything else work.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Some of the logic is similar but enough of it is different that it gets a little bit messy, as the snippet shows.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/3616d27c_296621cf">Patch Set #12, Line 889:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> char *files = ast_str_buffer(filenames);<br><br> while ((fn = strsep(&files, "&"))) {<br> res = ast_streamfile(chan, fn, language);<br> if (!res) {<br> if ((audiofd > -1) && (ctrlfd > -1))<br> res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);<br> else<br> res = ast_waitstream(chan, ints);<br> }<br> ast_stopstream(chan);<br> }<br><br> ast_free(filenames);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Duped code (5/6), combine into a single function</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16226/comment/fc53911b_146965b6">Patch Set #12, Line 914:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> char *files = ast_str_buffer(filenames);<br><br> while ((fn = strsep(&files, "&"))) {<br> res = ast_streamfile(chan, fn, language);<br> if (!res) {<br> if ((audiofd > -1) && (ctrlfd > -1))<br> res = ast_waitstream_full(chan, ints, audiofd, ctrlfd);<br> else<br> res = ast_waitstream(chan, ints);<br> }<br> ast_stopstream(chan);<br> }<br><br> ast_free(filenames);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Duped code (6/6), combine into a single function</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16226">change 16226</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/16226"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: If9718c89353b8e153d84add3cc4637b79585db19 </div>
<div style="display:none"> Gerrit-Change-Number: 16226 </div>
<div style="display:none"> Gerrit-PatchSet: 13 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-CC: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Attention: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 19 Aug 2021 20:15:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>