[PATCH] xfsprogs: mkfs: make strict check on -ialign option

[PATCH] xfsprogs: mkfs: make strict check on -ialign option

[Wengang Wang]

Though it's clearly said in mkfs.xfs man page that for -ialign option only 1 or
0 are valid values, I would like to make a strict check on it in code.

If a user specified -ialign=y(but he meant -ialign=1 actually), mkfs treats "y"
as "0"(simply by atoi()) thus acts wrongly without complaint. I think we'd better
prevent that from happening, so I made the patch. The patch fails the operation
on values for -ialign option, like "yes", "no", "y", "n".

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 mkfs/xfs_mkfs.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2d09e36..d7e9eb3 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1180,14 +1180,17 @@ main(
 			p = optarg;
 			while (*p != '\0') {
 				char	*value;
+				int len;
 
 				switch (getsubopt(&p, (constpp)iopts, &value)) {
 				case I_ALIGN:
 					if (!value)
 						value = "1";
-					iaflag = atoi(value);
-					if (iaflag < 0 || iaflag > 1)
+					len = strlen(value);
+					if (len != 1 || value[0] < '0' ||
+					    value[0] > '1')
 						illegal(value, "i align");
+					iaflag = value[0] - '0';
 					break;
 				case I_LOG:
 					if (!value)
-- 
1.6.6.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: mkfs: make strict check on -ialign option

[Dave Chinner]

On Mon, Apr 19, 2010 at 05:41:36PM +0800, Wengang Wang wrote:
> Though it's clearly said in mkfs.xfs man page that for -ialign option only 1 or
> 0 are valid values, I would like to make a strict check on it in code.
> 
> If a user specified -ialign=y(but he meant -ialign=1 actually), mkfs treats "y"
> as "0"(simply by atoi()) thus acts wrongly without complaint. I think we'd better
> prevent that from happening, so I made the patch. The patch fails the operation
> on values for -ialign option, like "yes", "no", "y", "n".
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  mkfs/xfs_mkfs.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2d09e36..d7e9eb3 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1180,14 +1180,17 @@ main(
>  			p = optarg;
>  			while (*p != '\0') {
>  				char	*value;
> +				int len;
>  
>  				switch (getsubopt(&p, (constpp)iopts, &value)) {
>  				case I_ALIGN:
>  					if (!value)
>  						value = "1";
> -					iaflag = atoi(value);
> -					if (iaflag < 0 || iaflag > 1)
> +					len = strlen(value);
> +					if (len != 1 || value[0] < '0' ||
> +					    value[0] > '1')
>  						illegal(value, "i align");
> +					iaflag = value[0] - '0';

Wouldn't this be better changing atoi() to strtol() and then checking
errno along with the bounds?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: mkfs: make strict check on -ialign option

[Wengang Wang]

On 10-04-20 13:45, Dave Chinner wrote:
> On Mon, Apr 19, 2010 at 05:41:36PM +0800, Wengang Wang wrote:
> > Though it's clearly said in mkfs.xfs man page that for -ialign option only 1 or
> > 0 are valid values, I would like to make a strict check on it in code.
> > 
> > If a user specified -ialign=y(but he meant -ialign=1 actually), mkfs treats "y"
> > as "0"(simply by atoi()) thus acts wrongly without complaint. I think we'd better
> > prevent that from happening, so I made the patch. The patch fails the operation
> > on values for -ialign option, like "yes", "no", "y", "n".
> > 
> > Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> > ---
> >  mkfs/xfs_mkfs.c |    7 +++++--
> >  1 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 2d09e36..d7e9eb3 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -1180,14 +1180,17 @@ main(
> >  			p = optarg;
> >  			while (*p != '\0') {
> >  				char	*value;
> > +				int len;
> >  
> >  				switch (getsubopt(&p, (constpp)iopts, &value)) {
> >  				case I_ALIGN:
> >  					if (!value)
> >  						value = "1";
> > -					iaflag = atoi(value);
> > -					if (iaflag < 0 || iaflag > 1)
> > +					len = strlen(value);
> > +					if (len != 1 || value[0] < '0' ||
> > +					    value[0] > '1')
> >  						illegal(value, "i align");
> > +					iaflag = value[0] - '0';
> 
> Wouldn't this be better changing atoi() to strtol() and then checking
> errno along with the bounds?
Hi Dave,

I tested strtol() with some cases, result is like the following:

<str> -- <result> errno: <errno>

"12" -- 12 errno: 0
"yes" -- 0 errno: 0
"no" -- 0 errno: 0
"y" -- 0 errno: 0
"" -- 0 errno: 0
"0" -- 0 errno: 0
"1" -- 1 errno: 0

It can't tell the original string is "yes", "no", "y", "" or "0" for
returning zero with errno being "OK". That is not the result I want.

regards,
wengang.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs