[lttng-dev] [PATCH lttng-tools] utils: Rework utils_parse_size_suffix
David Goulet
dgoulet at efficios.com
Thu Apr 10 15:57:51 EDT 2014
On 10 Apr (11:30:19), Simon Marchi wrote:
> Ok, so there are a lot of problems with this function (sorry :|). Taking
> the regex road is probably to complicated for nothing, so here is a
> version without regexes.
>
> I added many test cases as suggested by Sandeep Chaudhary and Daniel
> Thibault. I tested on both Intel 32 and 64 bits.
>
> Would fix #633
Why are you not sure of that? :P
Can you explain *why* regex was a bad choice here (if the 633 ticket
does not already explain it).
Thanks!
David
>
> Signed-off-by: Simon Marchi <simon.marchi at polymtl.ca>
> ---
> src/common/utils.c | 136 +++++++++++++-----------------
> src/common/utils.h | 2 +-
> tests/unit/test_utils_parse_size_suffix.c | 54 +++++++++++-
> 3 files changed, 111 insertions(+), 81 deletions(-)
>
> diff --git a/src/common/utils.c b/src/common/utils.c
> index 31596dd..9326080 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -28,7 +28,6 @@
> #include <sys/types.h>
> #include <unistd.h>
> #include <inttypes.h>
> -#include <regex.h>
> #include <grp.h>
> #include <pwd.h>
>
> @@ -650,42 +649,10 @@ error:
> return ret;
> }
>
> -/**
> - * Prints the error message corresponding to a regex error code.
> - *
> - * @param errcode The error code.
> - * @param regex The regex object that produced the error code.
> - */
> -static void regex_print_error(int errcode, regex_t *regex)
> -{
> - /* Get length of error message and allocate accordingly */
> - size_t length;
> - char *buffer;
> -
> - assert(regex != NULL);
> -
> - length = regerror(errcode, regex, NULL, 0);
> - if (length == 0) {
> - ERR("regerror returned a length of 0");
> - return;
> - }
> -
> - buffer = zmalloc(length);
> - if (!buffer) {
> - ERR("regex_print_error: zmalloc failed");
> - return;
> - }
> -
> - /* Get and print error message */
> - regerror(errcode, regex, buffer, length);
> - ERR("regex error: %s\n", buffer);
> - free(buffer);
> -
> -}
>
> /**
> * Parse a string that represents a size in human readable format. It
> - * supports decimal integers suffixed by 'k', 'M' or 'G'.
> + * supports decimal integers suffixed by 'k', 'K', 'M' or 'G'.
> *
> * The suffix multiply the integer by:
> * 'k': 1024
> @@ -693,83 +660,94 @@ static void regex_print_error(int errcode, regex_t *regex)
> * 'G': 1024^3
> *
> * @param str The string to parse.
> - * @param size Pointer to a size_t that will be filled with the
> + * @param size Pointer to a uint64_t that will be filled with the
> * resulting size.
> *
> * @return 0 on success, -1 on failure.
> */
> LTTNG_HIDDEN
> -int utils_parse_size_suffix(char *str, uint64_t *size)
> +int utils_parse_size_suffix(const char * const str, uint64_t * const size)
> {
> - regex_t regex;
> int ret;
> - const int nmatch = 3;
> - regmatch_t suffix_match, matches[nmatch];
> - unsigned long long base_size;
> + uint64_t base_size;
> long shift = 0;
> + const char *str_end;
> + char *num_end;
>
> if (!str) {
> - return 0;
> - }
> -
> - /* Compile regex */
> - ret = regcomp(®ex, "^\\(0x\\)\\{0,1\\}[0-9][0-9]*\\([kKMG]\\{0,1\\}\\)$", 0);
> - if (ret != 0) {
> - regex_print_error(ret, ®ex);
> + DBG("utils_parse_size_suffix: received a NULL string.");
> ret = -1;
> goto end;
> }
>
> - /* Match regex */
> - ret = regexec(®ex, str, nmatch, matches, 0);
> - if (ret != 0) {
> + /* strtoull will accept a negative number, but we don't want to. */
> + if (strchr(str, '-') != NULL) {
> + DBG("utils_parse_size_suffix: invalid size string, should not contain '-'.");
> ret = -1;
> - goto free;
> + goto end;
> }
>
> - /* There is a match ! */
> + /* str_end will point to the \0 */
> + str_end = str + strlen(str);
> errno = 0;
> - base_size = strtoull(str, NULL, 0);
> + base_size = strtoull(str, &num_end, 0);
> if (errno != 0) {
> - PERROR("strtoull");
> + PERROR("utils_parse_size_suffix strtoull");
> ret = -1;
> - goto free;
> + goto end;
> }
>
> - /* Check if there is a suffix */
> - suffix_match = matches[2];
> - if (suffix_match.rm_eo - suffix_match.rm_so == 1) {
> - switch (*(str + suffix_match.rm_so)) {
> - case 'K':
> - case 'k':
> - shift = KIBI_LOG2;
> - break;
> - case 'M':
> - shift = MEBI_LOG2;
> - break;
> - case 'G':
> - shift = GIBI_LOG2;
> - break;
> - default:
> - ERR("parse_human_size: invalid suffix");
> - ret = -1;
> - goto free;
> - }
> + if (num_end == str) {
> + /* strtoull parsed nothing, not good. */
> + DBG("utils_parse_size_suffix: strtoull had nothing good to parse.\n");
> + ret = -1;
> + goto end;
> + }
> +
> + /* Check if a prefix is present. */
> + switch (*num_end) {
> + case 'G':
> + shift = GIBI_LOG2;
> + num_end++;
> + break;
> +
> + case 'M': /* */
> + shift = MEBI_LOG2;
> + num_end++;
> + break;
> +
> + case 'K':
> + case 'k':
> + shift = KIBI_LOG2;
> + num_end++;
> + break;
> +
> + case '\0':
> + break;
> +
> + default:
> + DBG("utils_parse_size_suffix: invalid suffix.\n");
> + ret = -1;
> + goto end;
> + }
> +
> + /* Check for garbage after the valid input. */
> + if (num_end != str_end) {
> + DBG("utils_parse_size_suffix: Garbage after size string.");
> + ret = -1;
> + goto end;
> }
>
> *size = base_size << shift;
>
> /* Check for overflow */
> if ((*size >> shift) != base_size) {
> - ERR("parse_size_suffix: oops, overflow detected.");
> + DBG("utils_parse_size_suffix: oops, overflow detected.");
> ret = -1;
> - goto free;
> + goto end;
> }
>
> ret = 0;
> -
> -free:
> - regfree(®ex);
> end:
> return ret;
> }
> diff --git a/src/common/utils.h b/src/common/utils.h
> index 63290a7..b872b53 100644
> --- a/src/common/utils.h
> +++ b/src/common/utils.h
> @@ -43,7 +43,7 @@ int utils_create_stream_file(const char *path_name, char *file_name, uint64_t si
> int utils_rotate_stream_file(char *path_name, char *file_name, uint64_t size,
> uint64_t count, int uid, int gid, int out_fd, uint64_t *new_count,
> int *stream_fd);
> -int utils_parse_size_suffix(char *str, uint64_t *size);
> +int utils_parse_size_suffix(char const * const str, uint64_t * const size);
> int utils_get_count_order_u32(uint32_t x);
> char *utils_get_home_dir(void);
> char *utils_get_user_home_dir(uid_t uid);
> diff --git a/tests/unit/test_utils_parse_size_suffix.c b/tests/unit/test_utils_parse_size_suffix.c
> index 990aa1b..45a87b0 100644
> --- a/tests/unit/test_utils_parse_size_suffix.c
> +++ b/tests/unit/test_utils_parse_size_suffix.c
> @@ -43,11 +43,63 @@ static struct valid_test_input valid_tests_inputs[] = {
> { "0x1234k", 4771840 },
> { "32M", 33554432 },
> { "1024G", 1099511627776 },
> + { "0X400", 1024 },
> + { "0x40a", 1034 },
> + { "0X40b", 1035 },
> + { "0x40C", 1036 },
> + { "0X40D", 1037 },
> + { "0x40e", 1038 },
> + { "0X40f", 1039 },
> + { "00", 0 },
> + { "0k", 0 },
> + { "0K", 0 },
> + { "0M", 0 },
> + { "0G", 0 },
> + { "00k", 0 },
> + { "00K", 0 },
> + { "00M", 0 },
> + { "00G", 0 },
> + { "0x0", 0 },
> + { "0X0", 0 },
> + { "0x0k", 0 },
> + { "0X0K", 0 },
> + { "0x0M", 0 },
> + { "0X0G", 0 },
> + { "0X40G", 68719476736 },
> + { "0300k", 196608 },
> + { "0300K", 196608 },
> + { "030M", 25165824 },
> + { "020G", 17179869184 },
> + { "0xa0k", 163840 },
> + { "0xa0K", 163840 },
> + { "0XA0M", 167772160 },
> + { "0xA0G", 171798691840 },
> };
> static const int num_valid_tests = sizeof(valid_tests_inputs) / sizeof(valid_tests_inputs[0]);
>
> /* Invalid test cases */
> -static char *invalid_tests_inputs[] = { "", "-1", "k", "4611686018427387904G" };
> +static char *invalid_tests_inputs[] = {
> + "",
> + " ",
> + "-1",
> + "k",
> + "4611686018427387904G",
> + "0x40g",
> + "08",
> + "09",
> + "0x",
> + "x0",
> + "0xx0",
> + "07kK",
> + "0xk",
> + "0XM",
> + "0xG",
> + "0x0MM",
> + "0X0GG",
> + "0a",
> + "0B",
> +};
> +
> static const int num_invalid_tests = sizeof(invalid_tests_inputs) / sizeof(invalid_tests_inputs[0]);
>
> static void test_utils_parse_size_suffix(void)
> --
> 1.9.1
>
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 603 bytes
Desc: Digital signature
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20140410/0a445e04/attachment.sig>
More information about the lttng-dev
mailing list