[Asterisk-code-review] StatsD: Add res statsd compatibility (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: StatsD: Add res_statsd compatibility
......................................................................


Patch Set 3:

(3 comments)

https://gerrit.asterisk.org/#/c/1561/3/res/res_statsd.c
File res/res_statsd.c:

Line 118: 		ast_free(msg);
This is not necessary since msg has not been initialized at this point.


Line 128: 		ast_free(msg);
This is not necessary, because the if check here is testing whether msg was successfully allocated. A NULL return from ast_str_create() indicates an unsuccessful allocation, meaning it is not necessary to free msg.


Line 158: 	const char *char_value;
        : 
        : 	char_value = "%jd", value;
Unfortunately, C doesn't quite work this way, even though it would be really cool if it did.

There are two basic problems here.

The first is the easiest to fix. Doing
    char_value = "%jd", value;
will not actually create a string with the value expressed as a string. Instead, you need to make use of the snprintf() function to do what you want here.

The second problem is that C requires you to explicitly spell out the memory allocations when using pointers. There are a couple of ways you can do what you need to do.

The first way is to declare char_value as a static array.
    char char_value[30];
    snprintf(char_value, sizeof(char_value), "%jd", value);
    ast_statsd_log_string(metric_name, metric_type, char_value, sample_rate);

The second is to dynamically let the memory be allocated.
    char *char_value;
    if (ast_asprintf(&char_value, "%jd", value) < 0) {
        return;
    }
    ast_statsd_log_string(metric_name, metrid_type, char_value, sample_rate);
    ast_free(char_value);

The advantage to the first way is that there is no possibility of failing due to being out of memory, and it will run quicker since it is not having to make a system call to retrieve new memory each time. The second would be a good option if you were unsure of how large a particular string would be, and you want to be able to accommodate many string lengths. The first method caps you at a maximum of 30 characters in your string (29 really, since C requires a null byte at the end of the string).

For this particular application, since you are placing a numerical value into char_value, there is no worry about the size of what you are putting in since numbers are well-bounded. My example using 30 as the size will allow for much larger numbers than could actually be used, so you can feel free to just use that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id6bb53600943d27347d2bcae26c0bd5643567611
Gerrit-PatchSet: 3
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: Tyler Cambron <tcambron at digium.com>
Gerrit-HasComments: Yes