[PATCH] Improve hex parser v.2

[Peter Stuge]

Thanks for the initiative! I completely agree with adding this
functionality, but I have some concern about the implementation..


> +++ b/src/utils.c
..
> +int osmo_hexparse_adv(const char *str, uint8_t *b, int max_len, bool allow_odd)

What's the use case for not allowing an odd number of hexits?

I think if any code relies on osmo_hexparse() being unable to parse
an odd number of hexits then that code must be fixed.


>  {
> -	int i, l, v;
> +	int v;
> +	size_t i, l = strlen(str);
> +	const char * src;
> +
> +	if (l > 2) { /* remove 0x prefix if any */
> +	    if ('0' == str[0] && 'x' == str[1]) {

I suggest:

if (!strncmp(str, "0x", 2)) {


> +	if (allow_odd) {
> +	    if (l & 1) i = l + 1;
> +	    else i = l;

Is the above code style used elsewhere in the file? I haven't looked,
but the proposed lines can be made a lot nicer. Please optimize for
readability.


> +	    char hs[i]; /* enough space to store additional leading 0 if str length is odd */

Why is this even needed?


> +	    if (l & 1) {
> +		memcpy(hs + 1, str, l);
> +		hs[0] = '0';
> +	    } else {
> +		memcpy(hs, str, l);
> +	    }

Please don't ever add memcpy() to any code anywhere. Always try to
eliminate memcpy() everywhere.

There is no need for memcpy() here.

Just handle the case of odd number of hexits inside the loop instead.
It fits fine into the existing code paths. No memory allocation and
no memory copying needed. Always strive for zero copy.


> -	l = strlen(str);
> -	if ((l&1) || ((l>>1) > max_len))
> +	if ((l>>1) > max_len)
>  		return -1;

The old code also used bit shifts, but I'd prefer using l / 2 for
readability. The compiler will optimize it, and since this is not a
bitwise operation but an arithmetic operation it is more clear to
use the divide operand in the source code.

Bonus: divide operand takes precedence over greater-than condition,
so can remove parentheses.


>  	for (i=0; i<l; i++) {
> -		char c = str[i];
> +		char c = src[i];

Here, I suggest to instead insert something like:

if (0 == i && 1 == l % 2) {
  /* special case for odd number of hexits */
  /* convert one input character into lower nibble of output */
  continue;
}


//Peter