@henningw commented on this pull request.

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

In src/modules/slack/slack.c:

> +		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);

would be good to use strncpy, as you already have the len.

In src/modules/slack/slack.c:

> +	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);

snprintf could fail, so you should check for error (negative return value).

In src/modules/slack/slack.c:

> + */
+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);

see above

In src/modules/slack/doc/slack_admin.xml:

> +	<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:")

I think the parameter icon_emogi is mispelled, i think it should be named icon_emoji. Would be good to fix this in code and docs.

In src/modules/slack/slack.c:

> + * 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 },

I think the parameter icon_emogi is mispelled, i think it should be named icon_emoji. Would be good to fix this in code and docs.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.