<p> Attention is currently required from: Sean Bright, Mark Murawski. </p>
<p>Patch set 14:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/17655">View Change</a></p><p>10 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File funcs/func_groupcount.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/+/17655/comment/77eee47e_e2bad76b">Patch Set #11, Line 1385:</a> <code style="font-family:monospace,monospace"> ast_str_append(&out, 0, "%s,", ast_channel_name(gi->chan));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ah, is this new? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Additionally, given the snafu that has already happened here, I think a simple test for GROUP_CHANNEL_LIST would not hurt it.<br>The test should create a couple channels, assign them to the same group, and then verify the list (perhaps you set a global var in the right order). Critically there should not be a trailing comma at the end.<br>The same test or another could also verify that a group var can be set and retrieved.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File funcs/func_groupcount.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/+/17655/comment/68a0d2e3_7bea1588">Patch Set #14, Line 28:</a> <code style="font-family:monospace,monospace">#include <regex.h></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">By convention, asterisk.h always goes first.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Move regex.h to after asterisk.h but before the other includes.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/7f94e9f4_4d09f565">Patch Set #14, Line 593:</a> <code style="font-family:monospace,monospace"> buf[0] = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use '\0' for semantics, not 0</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/ecb12943_c34ed21c">Patch Set #14, Line 600:</a> <code style="font-family:monospace,monospace"> if (ast_app_group_split_group(args.groupcategory, group, sizeof(group), category, sizeof(category)))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This needs braces for single line ifs, per the coding guidelines</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/edf0c91f_372f5b87">Patch Set #14, Line 783:</a> <code style="font-family:monospace,monospace"> const char *group_match = NULL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Whitespace should be tabs here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/3d8f3f32_f2ac2039">Patch Set #14, Line 795:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Whitespace/alignment/indentation also messed up here</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/channel.h:</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/+/17655/comment/29ac0e3a_bf119274">Patch Set #14, Line 2931:</a> <code style="font-family:monospace,monospace"> struct varshead varshead; /*!< A linked list for group variables. See \ref AstGroupVar */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove one tab to align the comments</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/bfd6a133_6b0e4231">Patch Set #14, Line 3164:</a> <code style="font-family:monospace,monospace"> * \since 20</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think \since 20 is accurate and can be removed. This will likely go into 18 as well.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File main/app.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/+/17655/comment/99300a61_37dbb81a">Patch Set #14, Line 2433:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Whitespace issues again here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17655/comment/436fe9ab_2a4d975b">Patch Set #14, Line 2686:</a> <code style="font-family:monospace,monospace"> strncpy(gmi->group, group, (MAX_GROUP_LEN - 1));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Use ast_copy_string, not strncpy</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/17655">change 17655</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/+/17655"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 18 </div>
<div style="display:none"> Gerrit-Change-Id: I23e48d1cdfc8adaffdfec2e936e56143603914f2 </div>
<div style="display:none"> Gerrit-Change-Number: 17655 </div>
<div style="display:none"> Gerrit-PatchSet: 14 </div>
<div style="display:none"> Gerrit-Owner: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-Comment-Date: Sat, 17 Sep 2022 16:33:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Comment-In-Reply-To: Mark Murawski <markm@intellasoft.net> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>