[Asterisk-code-review] StatsD: Add user input validation to the application (asterisk[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: StatsD: Add user input validation to the application
......................................................................


Patch Set 2: Code-Review-1

(18 comments)

https://gerrit.asterisk.org/#/c/1445/2/apps/app_statsd.c
File apps/app_statsd.c:

Line 75: static const char app[] = "Statsd";
Name the application "StatsD", since that matches what the service is actually called.


Line 78: static int validate_metric(char* metric);
Remove the prototypes - they aren't needed. Place them before the definition of statsd_exec.


Line 104: 
Validation failure should emit a WARNING message.


Line 108: 	pbx_builtin_setvar_helper(chan, "STATSDSTATUS", "PASSED");
To match other dialplan applications, set "SUCCESS" on a nominal return instead of "PASSED".

Honestly, I'm not sure a dialplan variable is needed at all for this application, as a channel is merely hitting the application to go poke an external service, and the state of the channel should not be changed based on this application's success or failure. I'd consider removing it completely.


Line 122: 		int index;
Use i, not index.


Line 125: 		{
Curly braces don't belong on their own line.


Line 126: 			/*If none of the valid metrics matched the given metric and the 
Remove trailing white space.

Format your comment with a space between the comment delimiter and the text:

/* If none...


Line 128: 			if((strcmp(valid_metrics[index], metric) == 0)) {
Space between if and (:

if (strcmp(

You also have too many parenthesis here.

if (!strcmp(valid_metrics[i], metric)) {

}

You also should not compare to 0. While some places in the code base do that, it's not typical.


Line 129: 				break;
Don't break out of the loop here; just return 0.


Line 130: 			} else if (index == 3) {
Remove this. Let the failure case be the return at the bottom of the function.


Line 136: 	return 0;
This should return error by default, i.e., -1 (or 1).


Line 140: static int validate_name(char* name) {
Functions should have a curly brace on the next line.


Line 141: 	/*Check for an empty statistic name and the pipe (|) character, which is
Spacing here as well.


Line 143: 	if ((ast_strlen_zero(name)) || (strstr(name, "|") != NULL)) {
You don't need parenthesis around the invocation of ast_strlen_zero(name):

if (ast_strlen_zero(name) || (strstr(name, "|") != NULL)) {

}


Line 150: /*Check to ensure the value is valid.*/
Provide proper doxygen comments on your internal functions.


Line 151: static int validate_value(char* value) {
Curly brace on the next line


Line 152: 	/*Check to ensure the value field is not empty and is a digit.*/
Spacing.


Line 155: 	} else {
You don't need the else here. Reduce indentation by removing the else statement.


-- 
To view, visit https://gerrit.asterisk.org/1445
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I55c7ce44326a68ad6c5c1514b9575ac50f25bbc3
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Tyler Cambron <tcambron at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes