Re: [PATCH lttng-tools] utils: Rework utils_parse_size_suffix

Re: [PATCH lttng-tools] utils: Rework utils_parse_size_suffix

[Thibault, Daniel]

----------------------------------------------------------------------
Date: Thu, 10 Apr 2014 14:43:38 -0400
From: Simon Marchi <simon.marchi@polymtl.ca>

>>>>   Should be "[...] should not begin with '-' [...]" (although it is 
>>>> true the size string should not *contain*, the test is only for 
>>>> *begins with*)

> I meant for it to search in the whole string, and that is what I think strchr does.

Oops, my bad.

> [...]
> +       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;
> +       }
>
>>>>   "utils_parse_size_suffix: zero-length size string." would be clearer.  Also note that the preceding DBG strings did not end with '\n'.

> This test indeed will catch zero-length strings. However, if you pass "hello", or "k", strtoull will just parse 0 characters and return the value 0. This test is for these cases as well, which are not zero-length strings. I agree that it's not very clear, but I have nothing better...

How about " utils_parse_size_suffix: size string does not begin with a number."?

Daniel U. Thibault
Protection des systèmes et contremesures (PSC) | Systems Protection & Countermeasures (SPC)
Cyber sécurité pour les missions essentielles (CME) | Mission Critical Cyber Security (MCCS)
R & D pour la défense Canada - Valcartier (RDDC Valcartier) | Defence R&D Canada - Valcartier (DRDC Valcartier)
2459 route de la Bravoure
Québec QC  G3J 1X5
CANADA
Vox : (418) 844-4000 x4245
Fax : (418) 844-4538
NAC : 918V QSDJ <http://www.travelgis.com/map.asp?addr=918V%20QSDJ>
Gouvernement du Canada | Government of Canada
<http://www.valcartier.drdc-rddc.gc.ca/>

Re: [PATCH lttng-tools] utils: Rework utils_parse_size_suffix

[David Goulet]

[-- Attachment #1.1: Type: text/plain, Size: 8442 bytes --]

Merged Thanks.

David

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
> 
> Signed-off-by: Simon Marchi <simon.marchi@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(&regex, "^\\(0x\\)\\{0,1\\}[0-9][0-9]*\\([kKMG]\\{0,1\\}\\)$", 0);
> -	if (ret != 0) {
> -		regex_print_error(ret, &regex);
> +		DBG("utils_parse_size_suffix: received a NULL string.");
>  		ret = -1;
>  		goto end;
>  	}
>  
> -	/* Match regex */
> -	ret = regexec(&regex, 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(&regex);
>  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@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools] utils: Rework utils_parse_size_suffix

[Simon Marchi]

On 10 April 2014 15:57, David Goulet <dgoulet@efficios.com> wrote:
> 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

Would fix if merged :P

> Can you explain *why* regex was a bad choice here (if the 633 ticket
> does not already explain it).

I did not say it was a *bad* choice, but when I look at the old and
new function, the new one seems less cryptic (without the matches and
all). This is debatable.

> Thanks!
> David

Re: [PATCH lttng-tools] utils: Rework utils_parse_size_suffix

[David Goulet]

[-- Attachment #1.1: Type: text/plain, Size: 8574 bytes --]

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@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(&regex, "^\\(0x\\)\\{0,1\\}[0-9][0-9]*\\([kKMG]\\{0,1\\}\\)$", 0);
> -	if (ret != 0) {
> -		regex_print_error(ret, &regex);
> +		DBG("utils_parse_size_suffix: received a NULL string.");
>  		ret = -1;
>  		goto end;
>  	}
>  
> -	/* Match regex */
> -	ret = regexec(&regex, 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(&regex);
>  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@lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

[-- Attachment #2: Type: text/plain, Size: 155 bytes --]

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools] utils: Rework utils_parse_size_suffix

[Simon Marchi]

Thanks for the comments.

> @@ -693,83 +660,94 @@ static void regex_print_error(int errcode, regex_t *regex)
> [...]
> +       /* 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 end;
>         }
>

>>>>   Should be "[...] should not begin with '-' [...]" (although it is true the size string should not *contain*, the test is only for *begins with*)

I meant for it to search in the whole string, and that is what I think
strchr does.

> [...]
> +       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;
> +       }
>
>>>>   "utils_parse_size_suffix: zero-length size string." would be clearer.  Also note that the preceding DBG strings did not end with '\n'.

This test indeed will catch zero-length strings. However, iif you pass
"hello", or "k", strtoull will just parse 0 characters and return the
value 0. This test is for these cases as well, which are not
zero-length strings. I agree that it's not very clear, but I have
nothing better...

Re: [PATCH lttng-tools] utils: Rework utils_parse_size_suffix

[Thibault, Daniel]

----------------------------------------------------------------------
Date: Thu, 10 Apr 2014 11:30:19 -0400
From: Simon Marchi <simon.marchi@polymtl.ca>

+++ b/src/common/utils.c
@@ -650,42 +649,10 @@ error:
[...]
  * The suffix multiply the integer by:

>>>   Should be "The suffixes multiply the integer by:"

@@ -693,83 +660,94 @@ static void regex_print_error(int errcode, regex_t *regex)
[...]
+	/* 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 end;
 	}

>>>   Should be "[...] should not begin with '-' [...]" (although it is true the size string should not *contain*, the test is only for *begins with*)

[...]
+	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;
+	}

>>>   "utils_parse_size_suffix: zero-length size string." would be clearer.  Also note that the preceding DBG strings did not end with '\n'.

+	/* Check if a prefix is present. */

>>>   That should be "suffix".

[...]
+	default:
+		DBG("utils_parse_size_suffix: invalid suffix.\n");

>>>   Another DBG string with a superfluous trailing '\n'.

Daniel U. Thibault
Protection des systèmes et contremesures (PSC) | Systems Protection & Countermeasures (SPC)
Cyber sécurité pour les missions essentielles (CME) | Mission Critical Cyber Security (MCCS)
R & D pour la défense Canada - Valcartier (RDDC Valcartier) | Defence R&D Canada - Valcartier (DRDC Valcartier)
2459 route de la Bravoure
Québec QC  G3J 1X5
CANADA
Vox : (418) 844-4000 x4245
Fax : (418) 844-4538
NAC : 918V QSDJ <http://www.travelgis.com/map.asp?addr=918V%20QSDJ>
Gouvernement du Canada | Government of Canada
<http://www.valcartier.drdc-rddc.gc.ca/>

[PATCH lttng-tools] utils: Rework utils_parse_size_suffix

[Simon Marchi]

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

Signed-off-by: Simon Marchi <simon.marchi@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(&regex, "^\\(0x\\)\\{0,1\\}[0-9][0-9]*\\([kKMG]\\{0,1\\}\\)$", 0);
-	if (ret != 0) {
-		regex_print_error(ret, &regex);
+		DBG("utils_parse_size_suffix: received a NULL string.");
 		ret = -1;
 		goto end;
 	}
 
-	/* Match regex */
-	ret = regexec(&regex, 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(&regex);
 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