[PATCH] libxcmd: return error from cvtnum() on overflow

[PATCH] libxcmd: return error from cvtnum() on overflow

[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 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.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Note:
... should I change all callsites from "non-numeric" to "invalid" perhaps?

diff --git a/libxcmd/input.c b/libxcmd/input.c
index d7f29c1..6d4c003 100644
--- a/libxcmd/input.c
+++ b/libxcmd/input.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH V2] libxcmd: return error from cvtnum() on overflow

[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 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.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

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?

diff --git a/libxcmd/input.c b/libxcmd/input.c
index d7f29c1..c2bc4a0 100644
--- a/libxcmd/input.c
+++ b/libxcmd/input.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] libxcmd: return error from cvtnum() on overflow

[Alex Elder]

On Mon, 2011-02-28 at 15:26 -0600, Eric Sandeen wrote:
> 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.

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.

Reviewed-by: Alex Elder <aelder@sgi.com>

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH V2] libxcmd: return error from cvtnum() on overflow

[Eric Sandeen]

On 3/1/11 3:00 PM, Alex Elder wrote:
> On Mon, 2011-02-28 at 15:26 -0600, Eric Sandeen wrote:
>> 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.
> 
> 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.

> - 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?

I don't know ;)

> - 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.

> - 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.  :)

> - Call sites seem to be a bit varied on how (or whether) they
>   look for errors.  Kind of a mess...

yeah.

> 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

> Reviewed-by: Alex Elder <aelder@sgi.com>
> 
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs