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

15.01.2013 07:52, Sylvain Munaut пишет:

On Tue, Jan 15, 2013 at 1:59 AM, Peter Stuge peter@stuge.se wrote:

...

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

fyi, I've actually been discussing the path with him on IRC. Hopefully should get a new patch soon :p

Working on regression tests - stay tuned :)