14 Jan
2013
14 Jan
'13
6:22 p.m.
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.
--
best regards,
Max, http://fairwaves.ru
14 Jan
14 Jan
7:13 p.m.
New subject: [PATCH] Improve hex parser v.2
Second version:
- fix forgotten variable
- preserve backward compatibility with regards to odd length behavior
--
looking forward for your comments,
Max, http://fairwaves.ru
15 Jan
15 Jan
1:59 a.m.
New subject: [PATCH] Improve hex parser v.2
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
7:52 a.m.
New subject: [PATCH] Improve hex parser v.2
On Tue, Jan 15, 2013 at 1:59 AM, Peter Stuge <peter(a)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
10:56 a.m.
New subject: [PATCH] Improve hex parser v.2
15.01.2013 07:52, Sylvain Munaut пишет:
On Tue, Jan 15, 2013 at 1:59 AM, Peter Stuge
<peter(a)stuge.se> wrote:
Working on regression tests - stay tuned :)
--
best regards,
Max, http://fairwaves.ru
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
3:31 p.m.
New subject: [PATCH] Improve hex parser v.3
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 :)
--
best regards,
Max, http://fairwaves.ru
5:35 p.m.
New subject: [PATCH] Improve hex parser v.4
* Fixed last byte corruption
* Updated test suit
* Improved style
--
best regards,
Max, http://fairwaves.ru
4140
days inactive
4141
days old
baseband-devel@lists.osmocom.org
6 comments
3 participants
participants (3)
-
Peter Stuge -
Sylvain Munaut -
☎