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