[PATCH] t_stripealign: Fix fibmap error handling

[PATCH] t_stripealign: Fix fibmap error handling

[Carlos Maiolino]

FIBMAP only returns a negative value when the underlying filesystem does
not support FIBMAP or on permission error. For the remaining errors,
i.e. those usually returned from the filesystem itself, zero will be
returned.

We can not trust a zero return from the FIBMAP, and such behavior made
generic/223 succeed when it should not.

Also, we can't use perror() only to print errors when FIBMAP failed, or
it will simply print 'success' when a zero is returned.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 src/t_stripealign.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/t_stripealign.c b/src/t_stripealign.c
index 5cdadaae..164831f8 100644
--- a/src/t_stripealign.c
+++ b/src/t_stripealign.c
@@ -76,8 +76,11 @@ int main(int argc, char ** argv)
 		unsigned int	bmap = 0;
 
 		ret = ioctl(fd, FIBMAP, &bmap);
-		if (ret < 0) {
-			perror("fibmap");
+		if (ret <= 0) {
+			if (ret < 0)
+				perror("fibmap");
+			else
+				fprintf(stderr, "fibmap error\n");
 			free(fie);
 			close(fd);
 			return 1;
-- 
2.20.1

Re: [PATCH] t_stripealign: Fix fibmap error handling

[Darrick J. Wong]

On Fri, Aug 23, 2019 at 11:25:30AM +0200, Carlos Maiolino wrote:
> FIBMAP only returns a negative value when the underlying filesystem does
> not support FIBMAP or on permission error. For the remaining errors,
> i.e. those usually returned from the filesystem itself, zero will be
> returned.
> 
> We can not trust a zero return from the FIBMAP, and such behavior made
> generic/223 succeed when it should not.
> 
> Also, we can't use perror() only to print errors when FIBMAP failed, or
> it will simply print 'success' when a zero is returned.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  src/t_stripealign.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/t_stripealign.c b/src/t_stripealign.c
> index 5cdadaae..164831f8 100644
> --- a/src/t_stripealign.c
> +++ b/src/t_stripealign.c
> @@ -76,8 +76,11 @@ int main(int argc, char ** argv)
>  		unsigned int	bmap = 0;
>  
>  		ret = ioctl(fd, FIBMAP, &bmap);
> -		if (ret < 0) {
> -			perror("fibmap");
> +		if (ret <= 0) {
> +			if (ret < 0)
> +				perror("fibmap");
> +			else
> +				fprintf(stderr, "fibmap error\n");

"fibmap returned no result"?

--D

>  			free(fie);
>  			close(fd);
>  			return 1;
> -- 
> 2.20.1
> 

Re: [PATCH] t_stripealign: Fix fibmap error handling

[Eryu Guan]

On Fri, Aug 23, 2019 at 07:36:50AM -0700, Darrick J. Wong wrote:
> On Fri, Aug 23, 2019 at 11:25:30AM +0200, Carlos Maiolino wrote:
> > FIBMAP only returns a negative value when the underlying filesystem does
> > not support FIBMAP or on permission error. For the remaining errors,
> > i.e. those usually returned from the filesystem itself, zero will be
> > returned.
> > 
> > We can not trust a zero return from the FIBMAP, and such behavior made
> > generic/223 succeed when it should not.
> > 
> > Also, we can't use perror() only to print errors when FIBMAP failed, or
> > it will simply print 'success' when a zero is returned.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  src/t_stripealign.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/t_stripealign.c b/src/t_stripealign.c
> > index 5cdadaae..164831f8 100644
> > --- a/src/t_stripealign.c
> > +++ b/src/t_stripealign.c
> > @@ -76,8 +76,11 @@ int main(int argc, char ** argv)
> >  		unsigned int	bmap = 0;
> >  
> >  		ret = ioctl(fd, FIBMAP, &bmap);
> > -		if (ret < 0) {
> > -			perror("fibmap");
> > +		if (ret <= 0) {
> > +			if (ret < 0)
> > +				perror("fibmap");
> > +			else
> > +				fprintf(stderr, "fibmap error\n");
> 
> "fibmap returned no result"?

Fixed on commit. Thanks!

Eryu

Re: [PATCH] t_stripealign: Fix fibmap error handling

[Carlos Maiolino]

On Sun, Aug 25, 2019 at 09:41:54PM +0800, Eryu Guan wrote:
> On Fri, Aug 23, 2019 at 07:36:50AM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 23, 2019 at 11:25:30AM +0200, Carlos Maiolino wrote:
> > > FIBMAP only returns a negative value when the underlying filesystem does
> > > not support FIBMAP or on permission error. For the remaining errors,
> > > i.e. those usually returned from the filesystem itself, zero will be
> > > returned.
> > > 
> > > We can not trust a zero return from the FIBMAP, and such behavior made
> > > generic/223 succeed when it should not.
> > > 
> > > Also, we can't use perror() only to print errors when FIBMAP failed, or
> > > it will simply print 'success' when a zero is returned.
> > > 
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > ---
> > >  src/t_stripealign.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/t_stripealign.c b/src/t_stripealign.c
> > > index 5cdadaae..164831f8 100644
> > > --- a/src/t_stripealign.c
> > > +++ b/src/t_stripealign.c
> > > @@ -76,8 +76,11 @@ int main(int argc, char ** argv)
> > >  		unsigned int	bmap = 0;
> > >  
> > >  		ret = ioctl(fd, FIBMAP, &bmap);
> > > -		if (ret < 0) {
> > > -			perror("fibmap");
> > > +		if (ret <= 0) {
> > > +			if (ret < 0)
> > > +				perror("fibmap");
> > > +			else
> > > +				fprintf(stderr, "fibmap error\n");
> > 
> > "fibmap returned no result"?
> 
> Fixed on commit. Thanks!

TZ discrepancy. Thanks for fixing it on commit Eryu :)


> 
> Eryu

-- 
Carlos