Re: [ovs-dev] [PATCH] socket: Don't fail when AF_UNIX connect() returns EAGAIN.
On 4/27/24 00:42, Ihar Hrachyshka wrote:
> POSIX defines EINPROGRESS as the return value for non-blocking connect()
> [1]. But in Linux, AF_UNIX connect() returns EAGAIN instead of
> EINPROGRESS. (but only for AF_UNIX sockets!) [2]
>
> Both cases should be handled the same way - by returning the `fd` and
> letting the caller to complete connection asynchronously.
>
> [1]:
> https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/functions/connect.html
> [2]: see `connect(2)` on Linux for details
>
> Signed-off-by: Ihar Hrachyshka
> ---
Hi, Ihar. Thanks for the patch!
I was reading the original commit that introduced the error mangling:
commit 21a60ea97b492ae642d5b35430b24d137cd67f35
Author: Ben Pfaff
Date: Fri Dec 11 16:59:44 2009 -0800
socket-util: Clarify EAGAIN error code for make_unix_socket().
make_unix_socket() can return EAGAIN in rare circumstances, e.g. when the
server's socket listen queue is full. A lot of OVS callers interpret
EAGAIN as a "try again" error code, but in this case it means that the
attempt to create the socket failed. So munge EAGAIN into another error
code to prevent that misinterpretation.
IIUC, at the time there were more direct users for this function
that would not re-try the connect() and will remain in the broken
state. Most of them were migrated to the stream- and reconnect-
wrappers, so they will actually re-connect normally now. That's
should be fine.
There is still one direct user in syslog-direct.c, but it is using
a blocking connection, so I guess it can't fall into this condition.
So, the change is fine, but I think more information addressing the
reasoning from the original commit should be added to the commit
message.
And this patch is missing changes for the equivalent python code in
python/ovs/socket_util.py. We should keep them in sync.
> lib/socket-util-unix.c | 8 +--
> lib/socket-util.c | 2 +-
> lib/socket-util.h | 2 +
> tests/.gitignore| 1 +
> tests/automake.mk | 3 +-
> tests/library.at| 5 ++
> tests/test-unix-socket-listen-backlog.c | 73 +
> 7 files changed, 88 insertions(+), 6 deletions(-)
> create mode 100644 tests/test-unix-socket-listen-backlog.c
>
> diff --git a/lib/socket-util-unix.c b/lib/socket-util-unix.c
> index 59f63fcce..1543aa2e2 100644
> --- a/lib/socket-util-unix.c
> +++ b/lib/socket-util-unix.c
> @@ -363,7 +363,10 @@ make_unix_socket(int style, bool nonblock,
> error = make_sockaddr_un(connect_path, &un, &un_len, &dirfd,
> linkname);
> if (!error
> && connect(fd, (struct sockaddr*) &un, un_len)
> -&& errno != EINPROGRESS) {
> +/* POSIX connect() returns EINPROGRESS for all non-blocking
> + * sockets. Linux connect() returns either EAGAIN - for AF_UNIX
> + * sockets - or EINPROGRESS - for other families. Handle both. */
2 spaces between sentences.
> +&& errno != EINPROGRESS && errno != EAGAIN) {
> error = errno;
> }
> free_sockaddr_un(dirfd, linkname);
> @@ -376,9 +379,6 @@ make_unix_socket(int style, bool nonblock,
> return fd;
>
> error:
> -if (error == EAGAIN) {
> -error = EPROTO;
> -}
> if (bind_path) {
> fatal_signal_unlink_file_now(bind_path);
> }
> diff --git a/lib/socket-util.c b/lib/socket-util.c
> index 2d89fce85..bc9628b38 100644
> --- a/lib/socket-util.c
> +++ b/lib/socket-util.c
> @@ -760,7 +760,7 @@ inet_open_passive(int style, const char *target, int
> default_port,
> }
>
> /* Listen. */
> -if (style == SOCK_STREAM && listen(fd, 64) < 0) {
> +if (style == SOCK_STREAM && listen(fd, LISTEN_BACKLOG) < 0) {
> error = sock_errno();
> VLOG_ERR("%s: listen: %s", target, sock_strerror(error));
> goto error;
> diff --git a/lib/socket-util.h b/lib/socket-util.h
> index 4eec627e3..03219633d 100644
> --- a/lib/socket-util.h
> +++ b/lib/socket-util.h
> @@ -28,6 +28,8 @@
> #include
> #include
>
> +#define LISTEN_BACKLOG 64
Should this also be used in stream-unix.c ?
might also be better to define the same constant for python counterparts.
> +
> struct ds;
>
> int set_nonblocking(int fd);
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 3a8c45975..c32b2c7cc 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -66,6 +66,7 @@
> /test-timeval
> /test-type-props
> /test-unix-socket
> +/test-unix-socket-listen-backlog
> /test-util
> /test-uuid
> /test-uuidset
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 04f48f2d8..fc74cfa7d 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -493,7 +493,8 @@ tests_ovstest_SOURCES = \
>
> if !WIN32
> tests_ovstest_SOURCES += \
> - tests/test-unix-socket.c
> + tests/test-unix-socket.c \
> + tests/te
Re: [ovs-dev] [PATCH] socket: Don't fail when AF_UNIX connect() returns EAGAIN.
On Fri, Apr 26, 2024 at 10:42:36PM +, Ihar Hrachyshka wrote: > POSIX defines EINPROGRESS as the return value for non-blocking connect() > [1]. But in Linux, AF_UNIX connect() returns EAGAIN instead of > EINPROGRESS. (but only for AF_UNIX sockets!) [2] > > Both cases should be handled the same way - by returning the `fd` and > letting the caller to complete connection asynchronously. > > [1]: > https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/functions/connect.html > [2]: see `connect(2)` on Linux for details > > Signed-off-by: Ihar Hrachyshka Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] socket: Don't fail when AF_UNIX connect() returns EAGAIN.
POSIX defines EINPROGRESS as the return value for non-blocking connect()
[1]. But in Linux, AF_UNIX connect() returns EAGAIN instead of
EINPROGRESS. (but only for AF_UNIX sockets!) [2]
Both cases should be handled the same way - by returning the `fd` and
letting the caller to complete connection asynchronously.
[1]:
https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/functions/connect.html
[2]: see `connect(2)` on Linux for details
Signed-off-by: Ihar Hrachyshka
---
lib/socket-util-unix.c | 8 +--
lib/socket-util.c | 2 +-
lib/socket-util.h | 2 +
tests/.gitignore| 1 +
tests/automake.mk | 3 +-
tests/library.at| 5 ++
tests/test-unix-socket-listen-backlog.c | 73 +
7 files changed, 88 insertions(+), 6 deletions(-)
create mode 100644 tests/test-unix-socket-listen-backlog.c
diff --git a/lib/socket-util-unix.c b/lib/socket-util-unix.c
index 59f63fcce..1543aa2e2 100644
--- a/lib/socket-util-unix.c
+++ b/lib/socket-util-unix.c
@@ -363,7 +363,10 @@ make_unix_socket(int style, bool nonblock,
error = make_sockaddr_un(connect_path, &un, &un_len, &dirfd, linkname);
if (!error
&& connect(fd, (struct sockaddr*) &un, un_len)
-&& errno != EINPROGRESS) {
+/* POSIX connect() returns EINPROGRESS for all non-blocking
+ * sockets. Linux connect() returns either EAGAIN - for AF_UNIX
+ * sockets - or EINPROGRESS - for other families. Handle both. */
+&& errno != EINPROGRESS && errno != EAGAIN) {
error = errno;
}
free_sockaddr_un(dirfd, linkname);
@@ -376,9 +379,6 @@ make_unix_socket(int style, bool nonblock,
return fd;
error:
-if (error == EAGAIN) {
-error = EPROTO;
-}
if (bind_path) {
fatal_signal_unlink_file_now(bind_path);
}
diff --git a/lib/socket-util.c b/lib/socket-util.c
index 2d89fce85..bc9628b38 100644
--- a/lib/socket-util.c
+++ b/lib/socket-util.c
@@ -760,7 +760,7 @@ inet_open_passive(int style, const char *target, int
default_port,
}
/* Listen. */
-if (style == SOCK_STREAM && listen(fd, 64) < 0) {
+if (style == SOCK_STREAM && listen(fd, LISTEN_BACKLOG) < 0) {
error = sock_errno();
VLOG_ERR("%s: listen: %s", target, sock_strerror(error));
goto error;
diff --git a/lib/socket-util.h b/lib/socket-util.h
index 4eec627e3..03219633d 100644
--- a/lib/socket-util.h
+++ b/lib/socket-util.h
@@ -28,6 +28,8 @@
#include
#include
+#define LISTEN_BACKLOG 64
+
struct ds;
int set_nonblocking(int fd);
diff --git a/tests/.gitignore b/tests/.gitignore
index 3a8c45975..c32b2c7cc 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -66,6 +66,7 @@
/test-timeval
/test-type-props
/test-unix-socket
+/test-unix-socket-listen-backlog
/test-util
/test-uuid
/test-uuidset
diff --git a/tests/automake.mk b/tests/automake.mk
index 04f48f2d8..fc74cfa7d 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -493,7 +493,8 @@ tests_ovstest_SOURCES = \
if !WIN32
tests_ovstest_SOURCES += \
- tests/test-unix-socket.c
+ tests/test-unix-socket.c \
+ tests/test-unix-socket-listen-backlog.c
endif
if LINUX
diff --git a/tests/library.at b/tests/library.at
index d962e1b3f..fff92bc93 100644
--- a/tests/library.at
+++ b/tests/library.at
@@ -219,6 +219,11 @@ AT_CHECK([mkdir $longname || exit 77])
AT_CHECK([cd $longname && $PYTHON3 $abs_srcdir/test-unix-socket.py
../$longname/socket socket])
AT_CLEANUP
+AT_SETUP([unix socket, listen backlog])
+AT_SKIP_IF([test "$IS_WIN32" = "yes"])
+AT_CHECK([ovstest test-unix-socket-listen-backlog x x])
+AT_CLEANUP
+
AT_SETUP([ovs_assert])
if test "$IS_WIN32" = "yes"; then
exit_status=9
diff --git a/tests/test-unix-socket-listen-backlog.c
b/tests/test-unix-socket-listen-backlog.c
new file mode 100644
index 0..d1a73ba39
--- /dev/null
+++ b/tests/test-unix-socket-listen-backlog.c
@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2010, 2012, 2014 Nicira, Inc.
+ * Copyright (c) 2024 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+#undef NDEBUG
+#include "socket-util.h"
+#include
+#include
+#include
+#include "ovstest.h"
+#include "util.h"
+
+static void
+test_unix_socket_listen_backlog_main(int argc, char *argv[])
+{
+const char *servername;
+
