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

[Joshua Colp]

Joshua Colp has posted comments on this change.

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


Patch Set 8: Code-Review-1

(7 comments)

My comments are really around improving the log messages to make it a bit easier for someone encountering problems to understand what is happening.

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

Line 75: static int value_in_range(const char *value, const char *metric) {
{ should be on the next line


Line 80: 			ast_log(AST_LOG_WARNING, "Value out of range!\n");
       : 			return 1;
       : 		}
       : 	} else {
       : 		if (numerical_value < 0 || numerical_value > pow(2, 64)) {
       : 			ast_log(AST_LOG_WARNING, "Value out of range!\n");
What value was out of range?


Line 120: 	ast_log(AST_LOG_ERROR, "Invalid metric type.\n");
What was the metric type that was invalid?


Line 140: 		ast_log(AST_LOG_ERROR, "Statistic name is missing or contains a pipe (|)"
Output the statistic name for the user.


Line 174: 				ast_log(AST_LOG_ERROR, "Value argument only contains a sign operator.\n");
Show what the value is.


Line 207: 		ast_log(AST_LOG_ERROR, "Correct format is "
        : 			"StatsD(metric_type,statistic_name,value)\n");
Same here as I say down below. It should be obvious when a user looks, but being explicit here is better.


Line 218: 		ast_log(AST_LOG_WARNING, "Invalid parameters. Correct format is "
        : 			"StatsD(metric_type,statistic_name,value)\n");
As a user this doesn't immediately point me to the problem.


-- 
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: 8
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Tyler Cambron <tcambron at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes