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 <ihrac...@redhat.com>
> ---
Hi, Ihar. Thanks for the patch!
I was reading the original commit that introduced the error mangling:
commit 21a60ea97b492ae642d5b35430b24d137cd67f35
Author: Ben Pfaff <b...@ovn.org>
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 <netinet/in_systm.h>
> #include <netinet/ip.h>
>
> +#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/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])
Maybe s/x/sock/ ?
> +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 000000000..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 <config.h>
> +#undef NDEBUG
> +#include "socket-util.h"
> +#include <errno.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include "ovstest.h"
> +#include "util.h"
> +
> +static void
> +test_unix_socket_listen_backlog_main(int argc, char *argv[])
> +{
> + const char *servername;
> + const char *clientname;
> + int serversock;
> + int clientsocks[LISTEN_BACKLOG + 1];
> +
> + set_program_name(argv[0]);
> +
> + if (argc != 3) {
> + ovs_fatal(0, "usage: %s SERVERSOCKET CLIENTSOCKET", argv[0]);
> + }
> + servername = argv[1];
> + clientname = argv[2];
> +
> + signal(SIGALRM, SIG_DFL);
> + alarm(5);
> +
> + /* Create a listening socket under name 'serversocket'. */
> + serversock = make_unix_socket(SOCK_STREAM, false, servername, NULL);
> + if (serversock < 0) {
> + ovs_fatal(-serversock, "%s: bind failed", servername);
> + }
> + if (listen(serversock, 1)) {
Should it be LISTEN_BACKLOG instead of 1 ?
Otherwise the clientsocks array size is kind of arbitrary, i.e.
it doesn't need to be LISTEN_BACKLOG + 1.
> + ovs_fatal(errno, "%s: listen failed", servername);
> + }
> +
> + /* Connect to 'clientname' (which should be the same file, perhaps under
> a
> + * different name). Connect enough times to overflow listen backlog. The
> + * last attempt should succeed, even though listen backlog is full and
> + * connect() returns EAGAIN (on Linux) or EINPROGRESS (on POSIX). */
Double spaces.
> + for (int i = 0; i < sizeof(clientsocks) / sizeof(clientsocks[0]); i++) {
ARRAY_SIZE macro from util.h.
> + clientsocks[i] = make_unix_socket(SOCK_STREAM, true, NULL,
> clientname);
> + if (clientsocks[i] < 0) {
> + ovs_fatal(-clientsocks[i], "%s: connect failed", clientname);
> + }
> + }
> +
> + for (int i = 0; i < sizeof(clientsocks) / sizeof(clientsocks[0]); i++) {
ARRAY_SIZE
> + close(clientsocks[i]);
Maybe instead of closing we should accept all them and then wait
for connection completion with check_connection_completion() on
all of them before closing?
> + }
> + close(serversock);
> +}
> +
> +OVSTEST_REGISTER("test-unix-socket-listen-backlog",
> + test_unix_socket_listen_backlog_main);
We may need a similar test for python.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
