<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>