Hello.
Current implementation of osmo_hexparse have couple of limitations (which are undocumented btw):
* it refuses to parse odd-length numbers (e. g. 7abdf) * it fails while parsing 0x prefix (e. g. 0xdead)
Both could be worked around by upper-layer code of course but since both cases above are perfectly valid hexadecimal numbers I think they should be handled by osmo_hexparse internally without complicating life of library users.
Attached patch does just that. Please review and merge if it feels OK.
Second version:
- fix forgotten variable - preserve backward compatibility with regards to odd length behavior
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
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
Cheers,
Sylvain
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 :)
Attached is improved version of the patch per discussions on irc and ML:
* no more memory allocation * more readable code * regression tests
Please review and merge if it's ok :)
* Fixed last byte corruption * Updated test suit * Improved style
baseband-devel@lists.osmocom.org