<p></p>
<p><b>@henningw</b> commented on this pull request.</p>

<p>Thanks for the pull request. I just had a quick look and added a few comments, related to parameter naming and error checking.</p><hr>

<p>In <a href="https://github.com/kamailio/kamailio/pull/2838#discussion_r698633778">src/modules/slack/slack.c</a>:</p>
<pre style='color:#555'>> +            return -1;
+       }
+
+       if(strncmp(val, "https://hooks.slack.com", 23)) {
+               LM_ERR("slack invalid webhook url [%s]\n", val);
+               return -1;
+       }
+
+       // TODO: parse webhook to multiple channels? eg.: chan1=>https://AAA/BBB/CC, chan2=>...
+
+       slack_url = (char*)pkg_malloc(len + 1);
+       if (slack_url==NULL) {
+               PKG_MEM_ERROR;
+               return -1;
+       }
+       strcpy(slack_url, val);
</pre>
<p>would be good to use strncpy, as you already have the len.</p>

<hr>

<p>In <a href="https://github.com/kamailio/kamailio/pull/2838#discussion_r698635853">src/modules/slack/slack.c</a>:</p>
<pre style='color:#555'>> +    return;
+}
+
+/**
+ * send message with curl
+ * @return 0 on success, -1 on error
+ */
+static int _curl_send(const char* uri, str *post_data)
+{
+       int datasz;
+       char* send_data;
+       CURL *curl_handle;
+       CURLcode res;
+       // LM_DBG("sending to[%s]\n", uri);
+
+       datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon);
</pre>
<p>snprintf could fail, so you should check for error (negative return value).</p>

<hr>

<p>In <a href="https://github.com/kamailio/kamailio/pull/2838#discussion_r698636159">src/modules/slack/slack.c</a>:</p>
<pre style='color:#555'>> + */
+static int _curl_send(const char* uri, str *post_data)
+{
+       int datasz;
+       char* send_data;
+       CURL *curl_handle;
+       CURLcode res;
+       // LM_DBG("sending to[%s]\n", uri);
+
+       datasz = snprintf(NULL, 0, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon);
+       send_data = (char*)pkg_malloc((datasz+1)*sizeof(char));
+       if(send_data==NULL) {
+        LM_ERR("Error: can not allocate pkg memory [%d] bytes\n", datasz);
+        return -1;
+    }
+    snprintf(send_data, datasz+1, BODY_FMT, slack_channel, slack_username, post_data->s, slack_icon);
</pre>
<p>see above</p>

<hr>

<p>In <a href="https://github.com/kamailio/kamailio/pull/2838#discussion_r698637604">src/modules/slack/doc/slack_admin.xml</a>:</p>
<pre style='color:#555'>> +    <section id="slack.p.icon_emogi">
+               <title><varname>icon_emogi</varname> (str)</title>
+               <para>
+                       specify an emoji (using colon shortcodes, eg. :white_check_mark:)
+                       to use as the profile photo alongside the message.
+               </para>
+               <para>
+               <emphasis>
+                       Default value is :ghost:
+               </emphasis>
+               </para>
+               <example>
+               <title>Set <varname>icon_emogi</varname> parameter</title>
+               <programlisting format="linespecific">
+...
+modparam("slack", "icon_emogi", ":ghost:")
</pre>
<p>I think the parameter <em>icon_emogi</em> is mispelled, i think it should be named <em>icon_emoji</em>. Would be good to fix this in code and docs.</p>

<hr>

<p>In <a href="https://github.com/kamailio/kamailio/pull/2838#discussion_r698637743">src/modules/slack/slack.c</a>:</p>
<pre style='color:#555'>> + * Exported functions
+ */
+static cmd_export_t cmds[] = {
+       {"slack_send",                        (cmd_function)slack_send1,                      1, slack_fixup,  0, ANY_ROUTE},
+       {0, 0, 0, 0, 0, 0}
+};
+
+
+/**
+ * Exported parameters
+ */
+static param_export_t mod_params[] = {
+       { "slack_url",                        PARAM_STRING|USE_FUNC_PARAM, (void*)_slack_url_param},
+       { "channel",                  PARAM_STRING, &slack_channel }, // channel starts with #
+       { "username",                 PARAM_STRING, &slack_username },
+       { "icon_emogi",                       PARAM_STRING, &slack_icon },
</pre>
<p>I think the parameter <em>icon_emogi</em> is mispelled, i think it should be named <em>icon_emoji</em>. Would be good to fix this in code and docs.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/kamailio/kamailio/pull/2838#pullrequestreview-741842887">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABO7UZNJLQUKLQCC5N5FZULT7OXXVANCNFSM5DB7SIMA">unsubscribe</a>.<br />Triage notifications on the go with GitHub Mobile for <a href="https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675">iOS</a> or <a href="https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub">Android</a>.
<img src="https://github.com/notifications/beacon/ABO7UZMW5W4EMSLJ42PYTVTT7OXXVA5CNFSM5DB7SIMKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOFQ3Z7RY.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/kamailio/kamailio/pull/2838#pullrequestreview-741842887",
"url": "https://github.com/kamailio/kamailio/pull/2838#pullrequestreview-741842887",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>