| Message ID | 1442324925-29609-1-git-send-email-yuriy.kolerov@synopsys.com |
|---|---|
| State | New |
| Headers | show |
Hi Yuriy, can you explain why these patches are useful? Better standard conformance? Testsuite or runtime fixes? Thanks Waldemar Yuriy Kolerov wrote, > Signed-off-by: Yuriy Kolerov <yuriy.kolerov@synopsys.com> > --- > libc/sysdeps/linux/common/fallocate.c | 13 ++++++++----- > libc/sysdeps/linux/common/fallocate64.c | 11 +++++++---- > 2 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/libc/sysdeps/linux/common/fallocate.c b/libc/sysdeps/linux/common/fallocate.c > index b231226..97ac303 100644 > --- a/libc/sysdeps/linux/common/fallocate.c > +++ b/libc/sysdeps/linux/common/fallocate.c > @@ -12,6 +12,7 @@ > #include <fcntl.h> > #include <bits/kernel-features.h> > #include <stdint.h> > +#include <errno.h> > > #if defined __NR_fallocate > extern __typeof(fallocate) __libc_fallocate attribute_hidden; > @@ -26,17 +27,19 @@ int attribute_hidden __libc_fallocate(int fd, int mode, __off_t offset, __off_t > uint32_t zero = 0; > INTERNAL_SYSCALL_DECL(err); > ret = (int) (INTERNAL_SYSCALL(fallocate, err, 6, fd, mode, > - __LONG_LONG_PAIR (zero, off_low), > - __LONG_LONG_PAIR (zero, len_low))); > + __LONG_LONG_PAIR(zero, off_low), > + __LONG_LONG_PAIR(zero, len_low))); > # elif __WORDSIZE == 64 > INTERNAL_SYSCALL_DECL(err); > ret = (int) (INTERNAL_SYSCALL(fallocate, err, 4, fd, mode, offset, len)); > # else > # error your machine is neither 32 bit or 64 bit ... it must be magical > # endif > - if (unlikely(INTERNAL_SYSCALL_ERROR_P (ret, err))) > - return INTERNAL_SYSCALL_ERRNO (ret, err); > - return 0; > + if (unlikely(INTERNAL_SYSCALL_ERROR_P(ret, err))) { > + __set_errno(INTERNAL_SYSCALL_ERRNO(ret, err)); > + ret = -1; > + } > + return ret; > } > > # if defined __UCLIBC_LINUX_SPECIFIC__ && defined __USE_GNU > diff --git a/libc/sysdeps/linux/common/fallocate64.c b/libc/sysdeps/linux/common/fallocate64.c > index cf75693..d051d37 100644 > --- a/libc/sysdeps/linux/common/fallocate64.c > +++ b/libc/sysdeps/linux/common/fallocate64.c > @@ -13,6 +13,7 @@ > #include <fcntl.h> > #include <bits/kernel-features.h> > #include <stdint.h> > +#include <errno.h> > > #if defined __NR_fallocate > > @@ -26,10 +27,12 @@ int attribute_hidden __libc_fallocate64(int fd, int mode, __off64_t offset, > int ret; > INTERNAL_SYSCALL_DECL(err); > ret = (int) (INTERNAL_SYSCALL(fallocate, err, 6, fd, mode, > - OFF64_HI_LO (offset), OFF64_HI_LO (len))); > - if (unlikely(INTERNAL_SYSCALL_ERROR_P (ret, err))) > - return INTERNAL_SYSCALL_ERRNO (ret, err); > - return 0; > + OFF64_HI_LO(offset), OFF64_HI_LO(len))); > + if (unlikely(INTERNAL_SYSCALL_ERROR_P(ret, err))) { > + __set_errno(INTERNAL_SYSCALL_ERRNO(ret, err)); > + ret = -1; > + } > + return ret; > } > > # if defined __UCLIBC_LINUX_SPECIFIC__ && defined __USE_GNU > -- > 2.2.0 > > _______________________________________________ > uClibc mailing list > uClibc@uclibc.org > http://lists.busybox.net/mailman/listinfo/uclibc >
On September 16, 2015 9:20:31 PM GMT+02:00, Waldemar Brodkorb <wbx@openadk.org> wrote: >Hi Yuriy, > >can you explain why these patches are useful? posix_fallocate does not set errno, the non-standard, Linux fallocate does, according to its manpage (and returns -1 upon error). >Better standard conformance? Testsuite or runtime fixes? So at least the former. Should double check not to regress the posix variant.. ( or maybe that's a bug in the standard, didn't look yet). HTH, > >Thanks > Waldemar
Hi Waldemar and Bernhard. First of all sorry for the absent of the detailed explanation of my fixes. I'm going to send a second version of patches after our discussion. The problem is that fallocate does not pass LTP (Linux Test Project) which checks return code and errno after fallocate execution. Then I've found that fallocate in uClibc does not set errno thus I've decided to fix it according to Linux manpage. But thanks to Bernhard I've also found that posix_fallocate has another behavior: "posix_fallocate() returns zero on success, or an error number on failure. Note that errno is not set." And posix_fallocate in uClibc refers to fallocate (oops). So my first patch is invalid in this place: it's also necessary to change posix_fallocate and posix_fallocate64 to meet requirements of POSIX (make it don't touch errno and just return error code). As for the second patch - fallocate does sign extension incorrectly when 32 bit values are passed (offset and length). It just fills the second word by zeros. E.g. fallocate works incorrectly when offset or length is negative value - in this case kernel thinks that positive values are passed. So I'm going to prepare a second version of patches with better explanation and posix_fallocate fix. Regards, Yuriy Kolerov -----Original Message----- From: Bernhard Reutner-Fischer [mailto:rep.dot.nop@gmail.com] Sent: Thursday, September 17, 2015 12:52 AM To: Waldemar Brodkorb; Yuriy Kolerov Cc: uclibc@uclibc.org; Vineet.Gupta1@synopsys.com; Alexey.Brodkin@synopsys.com; Francois.Bedard@synopsys.com Subject: Re: [PATCH 1/2] libc: fix setting return value and errno in fallocate() On September 16, 2015 9:20:31 PM GMT+02:00, Waldemar Brodkorb <wbx@openadk.org> wrote: >Hi Yuriy, > >can you explain why these patches are useful? posix_fallocate does not set errno, the non-standard, Linux fallocate does, according to its manpage (and returns -1 upon error). >Better standard conformance? Testsuite or runtime fixes? So at least the former. Should double check not to regress the posix variant.. ( or maybe that's a bug in the standard, didn't look yet). HTH, > >Thanks > Waldemar
On Thu, Sep 17, 2015 at 03:56:55PM +0000, Yuriy Kolerov wrote: > Hi Waldemar and Bernhard. > > First of all sorry for the absent of the detailed explanation of my > fixes. I'm going to send a second version of patches after our > discussion. > > The problem is that fallocate does not pass LTP (Linux Test Project) > which checks return code and errno after fallocate execution. Then > I've found that fallocate in uClibc does not set errno thus I've > decided to fix it according to Linux manpage. But thanks to Bernhard > I've also found that posix_fallocate has another behavior: > "posix_fallocate() returns zero on success, or an error number on > failure. Note that errno is not set." And posix_fallocate in uClibc > refers to fallocate (oops). So my first patch is invalid in this > place: it's also necessary to change posix_fallocate and > posix_fallocate64 to meet requirements of POSIX (make it don't touch > errno and just return error code). Not-touching-errno is not a POSIX requirement. This is just a case of an over-aggressive man page documenting glibc-specific behavior that is not standard and should not be relied upon by applications. You can see here that there is no such requirement (by default any function in the standard is allowed to modify errno unless it's specified not to): http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html Rich
Hi Rich. fallocate must return 0 or -1. However posix_fallocate my return an error code. So I think it would be better to allow posix_fallocate change errno as fallocate does it and fix posix_fallocate return value according to POSIX. Regards, Yuriy Kolerov -----Original Message----- From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of Rich Felker Sent: Thursday, September 17, 2015 7:08 PM To: Yuriy Kolerov Cc: Bernhard Reutner-Fischer; Waldemar Brodkorb; uclibc@uclibc.org; Vineet.Gupta1@synopsys.com; Alexey.Brodkin@synopsys.com; Francois.Bedard@synopsys.com Subject: Re: [PATCH 1/2] libc: fix setting return value and errno in fallocate() On Thu, Sep 17, 2015 at 03:56:55PM +0000, Yuriy Kolerov wrote: > Hi Waldemar and Bernhard. > > First of all sorry for the absent of the detailed explanation of my > fixes. I'm going to send a second version of patches after our > discussion. > > The problem is that fallocate does not pass LTP (Linux Test Project) > which checks return code and errno after fallocate execution. Then > I've found that fallocate in uClibc does not set errno thus I've > decided to fix it according to Linux manpage. But thanks to Bernhard > I've also found that posix_fallocate has another behavior: > "posix_fallocate() returns zero on success, or an error number on > failure. Note that errno is not set." And posix_fallocate in uClibc > refers to fallocate (oops). So my first patch is invalid in this > place: it's also necessary to change posix_fallocate and > posix_fallocate64 to meet requirements of POSIX (make it don't touch > errno and just return error code). Not-touching-errno is not a POSIX requirement. This is just a case of an over-aggressive man page documenting glibc-specific behavior that is not standard and should not be relied upon by applications. You can see here that there is no such requirement (by default any function in the standard is allowed to modify errno unless it's specified not to): http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html Rich
On September 17, 2015 6:07:38 PM GMT+02:00, Rich Felker <dalias@libc.org> wrote: >On Thu, Sep 17, 2015 at 03:56:55PM +0000, Yuriy Kolerov wrote: >> Hi Waldemar and Bernhard. >> >> First of all sorry for the absent of the detailed explanation of my >> fixes. I'm going to send a second version of patches after our >> discussion. >> >> The problem is that fallocate does not pass LTP (Linux Test Project) >> which checks return code and errno after fallocate execution. Then >> I've found that fallocate in uClibc does not set errno thus I've >> decided to fix it according to Linux manpage. But thanks to Bernhard >> I've also found that posix_fallocate has another behavior: >> "posix_fallocate() returns zero on success, or an error number on >> failure. Note that errno is not set." And posix_fallocate in uClibc >> refers to fallocate (oops). So my first patch is invalid in this >> place: it's also necessary to change posix_fallocate and >> posix_fallocate64 to meet requirements of POSIX (make it don't touch >> errno and just return error code). > >Not-touching-errno is not a POSIX requirement. This is just a case of >an over-aggressive man page documenting glibc-specific behavior that >is not standard and should not be relied upon by applications. Right. > You can >see here that there is no such requirement (by default any function in >the standard is allowed to modify errno unless it's specified not to): Where's that written? http://pubs.opengroup.org/onlinepubs/9699919799/functions/errno.html posix_fallocate does not mention it so errno is unspecified after such a call, AFAICS. Thanks, > >http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html > >Rich
On September 17, 2015 6:16:48 PM GMT+02:00, Yuriy Kolerov <Yuriy.Kolerov@synopsys.com> wrote: >Hi Rich. > >fallocate must return 0 or -1. However posix_fallocate my return an >error code. So I think it would be better to allow posix_fallocate >change errno as fallocate does it and fix posix_fallocate return value >according to POSIX. fallocate () should be implemented on top of posix_fallocate, yes. TIA, > >Regards, >Yuriy Kolerov > > >-----Original Message----- >From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of Rich Felker >Sent: Thursday, September 17, 2015 7:08 PM >To: Yuriy Kolerov >Cc: Bernhard Reutner-Fischer; Waldemar Brodkorb; uclibc@uclibc.org; >Vineet.Gupta1@synopsys.com; Alexey.Brodkin@synopsys.com; >Francois.Bedard@synopsys.com >Subject: Re: [PATCH 1/2] libc: fix setting return value and errno in >fallocate() > >On Thu, Sep 17, 2015 at 03:56:55PM +0000, Yuriy Kolerov wrote: >> Hi Waldemar and Bernhard. >> >> First of all sorry for the absent of the detailed explanation of my >> fixes. I'm going to send a second version of patches after our >> discussion. >> >> The problem is that fallocate does not pass LTP (Linux Test Project) >> which checks return code and errno after fallocate execution. Then >> I've found that fallocate in uClibc does not set errno thus I've >> decided to fix it according to Linux manpage. But thanks to Bernhard >> I've also found that posix_fallocate has another behavior: >> "posix_fallocate() returns zero on success, or an error number on >> failure. Note that errno is not set." And posix_fallocate in uClibc >> refers to fallocate (oops). So my first patch is invalid in this >> place: it's also necessary to change posix_fallocate and >> posix_fallocate64 to meet requirements of POSIX (make it don't touch >> errno and just return error code). > >Not-touching-errno is not a POSIX requirement. This is just a case of >an over-aggressive man page documenting glibc-specific behavior that is >not standard and should not be relied upon by applications. You can see >here that there is no such requirement (by default any function in the >standard is allowed to modify errno unless it's specified not to): > >http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html > >Rich
On Thu, Sep 17, 2015 at 09:27:40PM +0200, Bernhard Reutner-Fischer wrote: > On September 17, 2015 6:16:48 PM GMT+02:00, Yuriy Kolerov <Yuriy.Kolerov@synopsys.com> wrote: > >Hi Rich. > > > >fallocate must return 0 or -1. However posix_fallocate my return an > >error code. So I think it would be better to allow posix_fallocate > >change errno as fallocate does it and fix posix_fallocate return value > >according to POSIX. > > fallocate () should be implemented on top of posix_fallocate, yes. I don't think this is possible. fallocate has an extra mode argument which posix_fallocate lacks. Rich
I think it's meant that posix_fallocate must use fallocate as it is implemented now.
int posix_fallocate(int fd, __off_t offset, __off_t len)
{
return __libc_fallocate(fd, 0, offset, len);
}
Regards,
Yuriy Kolerov
-----Original Message-----
From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of Rich Felker
Sent: Friday, September 18, 2015 12:53 AM
To: Bernhard Reutner-Fischer
Cc: Yuriy Kolerov; uclibc@uclibc.org; Vineet.Gupta1@synopsys.com; Alexey.Brodkin@synopsys.com; Francois.Bedard@synopsys.com
Subject: Re: [PATCH 1/2] libc: fix setting return value and errno in fallocate()
On Thu, Sep 17, 2015 at 09:27:40PM +0200, Bernhard Reutner-Fischer wrote:
> On September 17, 2015 6:16:48 PM GMT+02:00, Yuriy Kolerov <Yuriy.Kolerov@synopsys.com> wrote:
> >Hi Rich.
> >
> >fallocate must return 0 or -1. However posix_fallocate my return an
> >error code. So I think it would be better to allow posix_fallocate
> >change errno as fallocate does it and fix posix_fallocate return
> >value according to POSIX.
>
> fallocate () should be implemented on top of posix_fallocate, yes.
I don't think this is possible. fallocate has an extra mode argument which posix_fallocate lacks.
Rich
On September 17, 2015 11:52:55 PM GMT+02:00, Rich Felker <dalias@libc.org> wrote: >On Thu, Sep 17, 2015 at 09:27:40PM +0200, Bernhard Reutner-Fischer >wrote: >> On September 17, 2015 6:16:48 PM GMT+02:00, Yuriy Kolerov ><Yuriy.Kolerov@synopsys.com> wrote: >> >Hi Rich. >> > >> >fallocate must return 0 or -1. However posix_fallocate my return an >> >error code. So I think it would be better to allow posix_fallocate >> >change errno as fallocate does it and fix posix_fallocate return >value >> >according to POSIX. >> >> fallocate () should be implemented on top of posix_fallocate, yes. > >I don't think this is possible. fallocate has an extra mode argument >which posix_fallocate lacks. Right, didn't remember the mode arg. Thanks,
diff --git a/libc/sysdeps/linux/common/fallocate.c b/libc/sysdeps/linux/common/fallocate.c index b231226..97ac303 100644 --- a/libc/sysdeps/linux/common/fallocate.c +++ b/libc/sysdeps/linux/common/fallocate.c @@ -12,6 +12,7 @@ #include <fcntl.h> #include <bits/kernel-features.h> #include <stdint.h> +#include <errno.h> #if defined __NR_fallocate extern __typeof(fallocate) __libc_fallocate attribute_hidden; @@ -26,17 +27,19 @@ int attribute_hidden __libc_fallocate(int fd, int mode, __off_t offset, __off_t uint32_t zero = 0; INTERNAL_SYSCALL_DECL(err); ret = (int) (INTERNAL_SYSCALL(fallocate, err, 6, fd, mode, - __LONG_LONG_PAIR (zero, off_low), - __LONG_LONG_PAIR (zero, len_low))); + __LONG_LONG_PAIR(zero, off_low), + __LONG_LONG_PAIR(zero, len_low))); # elif __WORDSIZE == 64 INTERNAL_SYSCALL_DECL(err); ret = (int) (INTERNAL_SYSCALL(fallocate, err, 4, fd, mode, offset, len)); # else # error your machine is neither 32 bit or 64 bit ... it must be magical # endif - if (unlikely(INTERNAL_SYSCALL_ERROR_P (ret, err))) - return INTERNAL_SYSCALL_ERRNO (ret, err); - return 0; + if (unlikely(INTERNAL_SYSCALL_ERROR_P(ret, err))) { + __set_errno(INTERNAL_SYSCALL_ERRNO(ret, err)); + ret = -1; + } + return ret; } # if defined __UCLIBC_LINUX_SPECIFIC__ && defined __USE_GNU diff --git a/libc/sysdeps/linux/common/fallocate64.c b/libc/sysdeps/linux/common/fallocate64.c index cf75693..d051d37 100644 --- a/libc/sysdeps/linux/common/fallocate64.c +++ b/libc/sysdeps/linux/common/fallocate64.c @@ -13,6 +13,7 @@ #include <fcntl.h> #include <bits/kernel-features.h> #include <stdint.h> +#include <errno.h> #if defined __NR_fallocate @@ -26,10 +27,12 @@ int attribute_hidden __libc_fallocate64(int fd, int mode, __off64_t offset, int ret; INTERNAL_SYSCALL_DECL(err); ret = (int) (INTERNAL_SYSCALL(fallocate, err, 6, fd, mode, - OFF64_HI_LO (offset), OFF64_HI_LO (len))); - if (unlikely(INTERNAL_SYSCALL_ERROR_P (ret, err))) - return INTERNAL_SYSCALL_ERRNO (ret, err); - return 0; + OFF64_HI_LO(offset), OFF64_HI_LO(len))); + if (unlikely(INTERNAL_SYSCALL_ERROR_P(ret, err))) { + __set_errno(INTERNAL_SYSCALL_ERRNO(ret, err)); + ret = -1; + } + return ret; } # if defined __UCLIBC_LINUX_SPECIFIC__ && defined __USE_GNU
Signed-off-by: Yuriy Kolerov <yuriy.kolerov@synopsys.com> --- libc/sysdeps/linux/common/fallocate.c | 13 ++++++++----- libc/sysdeps/linux/common/fallocate64.c | 11 +++++++---- 2 files changed, 15 insertions(+), 9 deletions(-)