libxcmd: return error from cvtnum() on overflow
PATCH 1/1 libxcmd: return error from cvtnum() on overflow
Test 071 was failing in weird ways, partly because it was trying to pass in offsets larger than strtoll() could accept, which then silently returned LLONG_MAX instead. For DIO tests, this was unaligned, so we got unexpected (to me, anyay) alignment errors.
At least printing out the perror() makes this more obvious, but unfortunately we then get the somewhat odd output:
# xfs_io -f -d -c "pwrite 9223373136366403584 4096" /mnt/test/grrr cvtnum: Numerical result out of range non-numeric offset argument -- 9223373136366403584
Test 071 still fails, but at least it's a bit more obvious as to why.
---
Note:
... should I change all callsites from "non-numeric" to "invalid" perhaps?
libxcmd/input.c +4 -0
@@ -153,6 +153,10 @@ cvtnum(int c; i =strtoll (s, &sp,0 ); +if (errno ==ERANGE ) { +perror ("cvtnum" ); +return -1LL ; + }if (i ==0 && sp == s)return -1LL ;if (*sp =='\0' )
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/1 libxcmd: return error from cvtnum() on overflow
Test 071 was failing in weird ways, partly because it was trying to pass in offsets larger than strtoll() could accept, which then silently returned LLONG_MAX instead. For DIO tests, this was unaligned, so we got unexpected (to me, anyay) alignment errors.
At least printing out the perror() makes this more obvious, but unfortunately we then get the somewhat odd output:
# xfs_io -f -d -c "pwrite 9223373136366403584 4096" /mnt/test/grrr cvtnum: Numerical result out of range non-numeric offset argument -- 9223373136366403584
Test 071 still fails, but at least it's a bit more obvious as to why.
---
V2: zero errno first so we don't pick up a stale errno.
Note:
... should I change all callsites from "non-numeric" to "invalid" perhaps?
libxcmd/input.c +5 -0
@@ -152,7 +152,12 @@ cvtnum(char *sp;int c; + errno =0 ; i =strtoll (s, &sp,0 ); +if (errno ==ERANGE ) { +perror ("cvtnum" ); +return -1LL ; + }if (i ==0 && sp == s)return -1LL ;if (*sp =='\0' )
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 5 lines · Eric Sandeen
Test 071 was failing in weird ways, partly because it was trying to pass in offsets larger than
strtoll()could accept, which then silently returnedLLONG_MAXinstead. For DIO tests, this was unaligned, so we got unexpected (to me, anyay) alignment errors.
Quoted 8 lines
At least printing out the
perror()makes this more obvious, but unfortunately we then get the somewhat odd output:# xfs_io -f -d -c "pwrite 9223373136366403584 4096" /mnt/test/grrr cvtnum: Numerical result out of range non-numeric offset argument -- 9223373136366403584Test 071 still fails, but at least it's a bit more obvious as to why.
Your change looks good. But here are a few more general questions (for anyone who cares to respond--not just you):
- Do you plan to get test 071 working? (Just curious.)
- mkfs/xfs_mkfs.c and extimate/xfs_estimate.c each define their own version of the same function. Do you know why? Is there any reason we couldn't just have one?
- The three version of cvtnum() are each a bit different. Two of them (the other two) return -1 for an empty string, while this one returns 0.
- I'm not sure what you meant by "non-numeric" versus "invalid" in call sites.
- Call sites seem to be a bit varied on how (or whether) they look for errors. Kind of a mess...
Regardless, you can consider this one reviewed. We should fix all three instances of the function to fix this problem though--either the same as this (and in the same commit) or separeately.
Quoted 7 lines
Signed-off-by Eric Sandeen <[email protected]>---
V2: zero errno first so we don't pick up a stale errno.
Note:
... should I change all callsites from "non-numeric" to "invalid" perhaps?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 13 lines · Alex Elder
Quoted 12 lines · Eric Sandeen
Test 071 was failing in weird ways, partly because it was trying to pass in offsets larger than
strtoll()could accept, which then silently returnedLLONG_MAXinstead. For DIO tests, this was unaligned, so we got unexpected (to me, anyay) alignment errors.At least printing out the
perror()makes this more obvious, but unfortunately we then get the somewhat odd output:# xfs_io -f -d -c "pwrite 9223373136366403584 4096" /mnt/test/grrr cvtnum: Numerical result out of range non-numeric offset argument -- 9223373136366403584
Quoted 5 lines
Quoted 1 line
Test 071 still fails, but at least it's a bit more obvious as to why.
Your change looks good. But here are a few more general questions (for anyone who cares to respond--not just you):
- Do you plan to get test 071 working? (Just curious.)
some day maybe, and I'd like to make it a generic test.
Quoted 3 lines
-
mkfs/xfs_mkfs.candextimate/xfs_estimate.ceach define their own version of the same function. Do you know why? Is there any reason we couldn't just have one?
I don't know ;)
Quoted 3 lines
- The three version of
cvtnum()are each a bit different. Two of them (the other two) return -1 for an empty string, while this one returns 0.
hrm.
Quoted 2 lines
- I'm not sure what you meant by "non-numeric" versus "invalid" in call sites.
I mean perror says:
cvtnum: Numerical result out of range
but then the caller says:
non-numeric offset argument -- 9223373136366403584
"9223373136366403584" is not non-numeric; it is out of range. :)
Quoted 2 lines
- Call sites seem to be a bit varied on how (or whether) they look for errors. Kind of a mess...
yeah.
Quoted 4 lines
Regardless, you can consider this one reviewed. We should fix all three instances of the function to fix this problem though--either the same as this (and in the same commit) or separeately.
ok I may fix up the others, I'd forgotten about that.
-Eric
Quoted 11 lines
Reviewed-by Alex Elder <[email protected]>Quoted 7 lines
Signed-off-by Eric Sandeen <[email protected]>---
V2: zero errno first so we don't pick up a stale errno.
Note:
... should I change all callsites from "non-numeric" to "invalid" perhaps?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs