Re: [PATCH] accept4: Support SOCK_NONBLOCK, if defined
On 8/20/19 2:29 PM, Bruno Haible wrote: > Eric Blake wrote: >> +#if SOCK_CLOEXEC >> + if (flags & SOCK_NONBLOCK) > > Why '#if SOCK_CLOEXEC'? > Why not '#if SOCK_NONBLOCK'? Because I made a typo. Thanks for spotting it; will fix shortly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [PATCH] accept4: Support SOCK_NONBLOCK, if defined
Eric Blake wrote: > Hmm, I guess it would be better to make the accept4 module depend on > nonblocking I would not like this. The 'nonblocking' module is quite intrusive (it hooks into many stdio functions, from 'getchar' to 'fprintf'). Few applications use nonblocking file, pipe, or sockets. The majority of the applications should not be suffering from the complexities of this module. Bruno
Re: accept4 and SOCK_NONBLOCK
Eric Blake wrote: > #if HAVE_ACCEPT4 > > Aha - there's the bug. > > Commit e597d4b8 changed from AC_CHECK_FUNCS_ONCE (defines HAVE_ACCEPT4) > to AC_CHECK_DECLS (defines HAVE_DECL_ACCEPT4), but failed to update the > .c file. I'll push the obvious fix. Indeed. My mistake, sorry. Bruno
Re: [PATCH] accept4: Support SOCK_NONBLOCK, if defined
Eric Blake wrote: > +#if SOCK_CLOEXEC > + if (flags & SOCK_NONBLOCK) Why '#if SOCK_CLOEXEC'? Why not '#if SOCK_NONBLOCK'? Bruno
Re: [PATCH] accept4: Support SOCK_NONBLOCK, if defined
On Tue, Aug 20, 2019 at 11:37:27AM -0500, Eric Blake wrote:
> +#if SOCK_CLOEXEC
> + if (flags & SOCK_NONBLOCK)
> +{
> + int fcntl_flags;
> +
> + if ((fcntl_flags = fcntl (fd, F_GETFL, 0)) < 0
> + || fcntl (fd, F_SETFL, fcntl_flags | O_NONBLOCK) == -1)
Can't this call set_nonblocking_flag (fd)?
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
Re: [PATCH] accept4: Support SOCK_NONBLOCK, if defined
On 8/20/19 11:37 AM, Eric Blake wrote:
> Ideally, we would improve our replacement to define a
> replacement SOCK_NONBLOCK on all platforms, and teach socket() to
> honor it as well; but that's a bigger task. In the meantime, if the
> platform already has SOCK_NONBLOCK, we should honor it when doing a
> fallback.
>
>
> +#if SOCK_CLOEXEC
> + if (flags & SOCK_NONBLOCK)
> +{
> + int fcntl_flags;
> +
> + if ((fcntl_flags = fcntl (fd, F_GETFL, 0)) < 0
> + || fcntl (fd, F_SETFL, fcntl_flags | O_NONBLOCK) == -1)
Hmm, I guess it would be better to make the accept4 module depend on
nonblocking, and call set_nonblocking() instead of open-coding the
Unix-only solution (but this works for now as long as mingw lacks
SOCK_NONBLOCK natively, and as long as we don't have
declaring a replacement SOCK_NONBLOCK)...
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
[PATCH] accept4: Support SOCK_NONBLOCK, if defined
Ideally, we would improve our replacement to define a
replacement SOCK_NONBLOCK on all platforms, and teach socket() to
honor it as well; but that's a bigger task. In the meantime, if the
platform already has SOCK_NONBLOCK, we should honor it when doing a
fallback.
* lib/accept4.c (accept4): If SOCK_NONBLOCK is defined, honor it.
---
ChangeLog | 3 +++
lib/accept4.c | 21 -
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index 6253f9221..f5eda2fa3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
2019-08-20 Eric Blake
+ accept4: Support SOCK_NONBLOCK, if defined
+ * lib/accept4.c (accept4): If SOCK_NONBLOCK is defined, honor it.
+
accept4: Fix compilation when native accept4() exists.
Reported by Richard W.M. Jones in
https://lists.gnu.org/archive/html/bug-gnulib/2019-08/msg00029.html
diff --git a/lib/accept4.c b/lib/accept4.c
index c6e59c955..9ce78fa96 100644
--- a/lib/accept4.c
+++ b/lib/accept4.c
@@ -31,6 +31,9 @@
#ifndef SOCK_CLOEXEC
# define SOCK_CLOEXEC 0
#endif
+#ifndef SOCK_NONBLOCK
+# define SOCK_NONBLOCK 0
+#endif
int
accept4 (int sockfd, struct sockaddr *addr, socklen_t *addrlen, int flags)
@@ -58,7 +61,7 @@ accept4 (int sockfd, struct sockaddr *addr, socklen_t
*addrlen, int flags)
#endif
/* Check the supported flags. */
- if ((flags & ~(SOCK_CLOEXEC | O_TEXT | O_BINARY)) != 0)
+ if ((flags & ~(SOCK_CLOEXEC | SOCK_NONBLOCK | O_TEXT | O_BINARY)) != 0)
{
errno = EINVAL;
return -1;
@@ -121,6 +124,22 @@ accept4 (int sockfd, struct sockaddr *addr, socklen_t
*addrlen, int flags)
# endif
#endif
+#if SOCK_CLOEXEC
+ if (flags & SOCK_NONBLOCK)
+{
+ int fcntl_flags;
+
+ if ((fcntl_flags = fcntl (fd, F_GETFL, 0)) < 0
+ || fcntl (fd, F_SETFL, fcntl_flags | O_NONBLOCK) == -1)
+{
+ int saved_errno = errno;
+ close (fd);
+ errno = saved_errno;
+ return -1;
+}
+}
+#endif
+
#if O_BINARY
if (flags & O_BINARY)
set_binary_mode (fd, O_BINARY);
--
2.21.0
Re: accept4 and SOCK_NONBLOCK
On 8/20/19 10:27 AM, Richard W.M. Jones wrote:
> First of all I'm using Linux 5.0.17 & glibc-2.29-15 so as far as I'm
> aware accept4 exists and fully works and gnulib shouldn't be replacing
> it at all. I don't understand why it's being replaced.
>
I think I'm reproducing your setup (Fedora 30), and running:
./gnulib-tool --test accept4
shows:
...
/usr/bin/mkdir -p sys
rm -f sys/socket.h-t sys/socket.h && \
{ echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
sed -e 's|@''GUARD_PREFIX''@|GL|g' \
...
-e 's/@''GNULIB_ACCEPT4''@/1/g' \
...
-e 's|@''HAVE_ACCEPT4''@|1|g' \
...
make[4]: Entering directory '/home/eblake/gnulib/testdir935/build/gllib'
gcc -DHAVE_CONFIG_H -I. -I../../gllib -I.. -DGNULIB_STRICT_CHECKING=1
-g -O2 -MT accept4.o -MD -MP -MF .deps/accept4.Tpo -c -o accept4.o
../../gllib/accept4.c
so it is indeed locating the system accept4(), but also compiling the
gnulib file. In turn, the gnulib file starts out:
#if HAVE_ACCEPT4
Aha - there's the bug.
Commit e597d4b8 changed from AC_CHECK_FUNCS_ONCE (defines HAVE_ACCEPT4)
to AC_CHECK_DECLS (defines HAVE_DECL_ACCEPT4), but failed to update the
.c file. I'll push the obvious fix.
> But given that, in libguestfs we call:
>
> r = accept4 (console_sock, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC);
>
>
> https://github.com/libguestfs/libguestfs/blob/master/lib/launch-libvirt.c#L639
>
> This is a valid set of flags according to the Linux man page for
> accept4(2). But it fails with EINVAL because of the following test in
> gnulib:
>
> if ((flags & ~(SOCK_CLOEXEC | O_TEXT | O_BINARY)) != 0)
> {
> errno = EINVAL;
> return -1;
> }
>
> https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/accept4.c#n61
Yes, that code is unnecessarily strict; gnulib should be taught to
handle SOCK_NONBLOCK, but doing it everywhere correctly is a bigger
task, as it also affects the socket() function.
>
> So I think the set of flags should be broadened to include
> SOCK_NONBLOCK + add a call to set_nonblocking_flag.
>
> As for why accept4 is being replaced at all, the only reference to it
> in config.log is:
>
> configure:25630: checking whether accept4 is declared
> configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security
> -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions
> -fstack-protector-strong -grecord-gcc-switches
> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
> -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic
> -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection
> conftest.c >&5
> configure:25630: $? = 0
> configure:25630: result: yes
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: accept4 and SOCK_NONBLOCK
* Richard W. M. Jones: > On Tue, Aug 20, 2019 at 05:37:58PM +0200, Florian Weimer wrote: >> * Richard W. M. Jones: >> >> > As for why accept4 is being replaced at all, the only reference to it >> > in config.log is: >> > >> > configure:25630: checking whether accept4 is declared >> > configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security >> > -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions >> > -fstack-protector-strong -grecord-gcc-switches >> > -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 >> > -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic >> > -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection >> > conftest.c >&5 >> > configure:25630: $? = 0 >> > configure:25630: result: yes >> >> What does the test program look like? Does it include ? >> accept4 requires _GNU_SOURCE. > > I'm not sure how to find out. However m4/accept.m4 (supplied > by gnulib itself) does contain: > > dnl Persuade glibc to declare accept4(). > AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS]) > > so I guess the test *ought* to be using _GNU_SOURCE. And the configure test acctually succeeded, which I missed. So it has to be something else. You'll have to investigate the generated files to see what's going wrong there. Thanks, Florian
Re: accept4 and SOCK_NONBLOCK
On Tue, Aug 20, 2019 at 05:37:58PM +0200, Florian Weimer wrote: > * Richard W. M. Jones: > > > As for why accept4 is being replaced at all, the only reference to it > > in config.log is: > > > > configure:25630: checking whether accept4 is declared > > configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security > > -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions > > -fstack-protector-strong -grecord-gcc-switches > > -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 > > -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic > > -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection > > conftest.c >&5 > > configure:25630: $? = 0 > > configure:25630: result: yes > > What does the test program look like? Does it include ? > accept4 requires _GNU_SOURCE. I'm not sure how to find out. However m4/accept.m4 (supplied by gnulib itself) does contain: dnl Persuade glibc to declare accept4(). AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS]) so I guess the test *ought* to be using _GNU_SOURCE. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Re: accept4 and SOCK_NONBLOCK
* Richard W. M. Jones: > As for why accept4 is being replaced at all, the only reference to it > in config.log is: > > configure:25630: checking whether accept4 is declared > configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security > -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions > -fstack-protector-strong -grecord-gcc-switches > -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 > -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic > -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection > conftest.c >&5 > configure:25630: $? = 0 > configure:25630: result: yes What does the test program look like? Does it include ? accept4 requires _GNU_SOURCE. Thanks, Florian
accept4 and SOCK_NONBLOCK
First of all I'm using Linux 5.0.17 & glibc-2.29-15 so as far as I'm
aware accept4 exists and fully works and gnulib shouldn't be replacing
it at all. I don't understand why it's being replaced.
But given that, in libguestfs we call:
r = accept4 (console_sock, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC);
https://github.com/libguestfs/libguestfs/blob/master/lib/launch-libvirt.c#L639
This is a valid set of flags according to the Linux man page for
accept4(2). But it fails with EINVAL because of the following test in
gnulib:
if ((flags & ~(SOCK_CLOEXEC | O_TEXT | O_BINARY)) != 0)
{
errno = EINVAL;
return -1;
}
https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/accept4.c#n61
So I think the set of flags should be broadened to include
SOCK_NONBLOCK + add a call to set_nonblocking_flag.
As for why accept4 is being replaced at all, the only reference to it
in config.log is:
configure:25630: checking whether accept4 is declared
configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security
-Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions
-fstack-protector-strong -grecord-gcc-switches
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1
-specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic
-fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection
conftest.c >&5
configure:25630: $? = 0
configure:25630: result: yes
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
