diff mbox series

t_stripealign: Fix fibmap error handling

Message ID 20190823092530.11797-1-cmaiolino@redhat.com (mailing list archive)
State Accepted, archived
Headers show
Series t_stripealign: Fix fibmap error handling | expand

Commit Message

Carlos Maiolino Aug. 23, 2019, 9:25 a.m. UTC
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(-)

Comments

Darrick J. Wong Aug. 23, 2019, 2:36 p.m. UTC | #1
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
>
Eryu Guan Aug. 25, 2019, 1:41 p.m. UTC | #2
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
Carlos Maiolino Aug. 26, 2019, 6:48 a.m. UTC | #3
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
diff mbox series

Patch

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;