Re: [PATCH] sctp: if backlog is 0, listening shall not be deactivated.

Previous thread: [RFC PATCH] perf_counter: Add support for pinned and exclusive counter groups by Paul Mackerras on Wednesday, January 14, 2009 - 2:24 am. (7 messages)

Next thread: Re: lib/klist.c: bit 0 in pointer can't be used as flag by Jesper Nilsson on Wednesday, January 14, 2009 - 3:19 am. (8 messages)
From: Pierre Habouzit
Date: Wednesday, January 14, 2009 - 2:42 am

POSIX hints that when 0 is used for the listen backlog argument, the
kernel shall chose a default automatic value. TCP for example, works this
way.

Signed-off-by: Pierre Habouzit <pierre.habouzit@intersec.com>
---

 net/sctp/socket.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

    To put a bit of background, I stumbled against this while doing a code
    that basically did:

	struct sctp_event_subscribe events;
	/* ... */

	fd = socket(AF_INET, SOCK_SEQPACKETS, IPPROTO_SCTP);
	sctp_bindx(fd, ....);
	events = (struct sctp_event_subscribe){
	    .sctp_data_io_event     = 1,
	    .sctp_association_event = 1,
	};
	setsockopt(fd, SOL_SCTP, SCTP_EVENTS, &events, sizeof(events));
	listen(fd, 0);
	len = sctp_recvmsg(fd, .....);

    The latter call instead of blocking like I expected returned with
    errno == ENOTCONN.

    I know POSIX doesn't _require_ listen() to accept 0 as a valid backlog,
    but the other listen() implementation I have used in the kernel do, and
    it looks really surprising for the programmer (who really searches the
    error elsewhere).

    Fortunately I had another working code at hand and I managed to find the
    problem resorting to reverting the changes I made to the original code
    line per line (*sigh*).

    I'm unsure if the diff shouldn't do instead:

    if (!backlog)
        backlog = 1;

    I'm not really comfortable around the kernel core ;)


diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index ff0a8f8..da1d96a 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5866,16 +5866,6 @@ SCTP_STATIC int sctp_seqpacket_listen(struct sock *sk, int backlog)
 	if (!sctp_style(sk, UDP))
 		return -EINVAL;
 
-	/* If backlog is zero, disable listening. */
-	if (!backlog) {
-		if (sctp_sstate(sk, CLOSED))
-			return 0;
-
-		sctp_unhash_endpoint(ep);
-		sk->sk_state = SCTP_SS_CLOSED;
-		return 0;
-	}
-
 	/* Return if we are already listening. */
 	if (sctp_sstate(sk, ...
From: Vlad Yasevich
Date: Wednesday, January 14, 2009 - 6:17 am

However SCTP API explicitly states that when the backlog is 0, listening is
disabled.  Here is an excerpt from the draft describing this:

   int listen(int sd,
              int backlog);

   and the arguments are
   sd:  The socket descriptor of the endpoint.
   backlog:  If backlog is non-zero, enable listening else disable
      listening.


So, this is something that spelled out in the draft.

NAK.


--

From: Alan Cox
Date: Wednesday, January 14, 2009 - 6:21 am

POSIX is an established standard, SCTP is a draft proposal. POSIX should
win. The SCTP developers need to bring their draft API into alignment with
POSIX.

They need to fix their draft to use a sockopt or similar to
enable/disable listening.

--

From: Vlad Yasevich
Date: Wednesday, January 14, 2009 - 6:36 am

Here is what POSIX says:

A backlog argument of 0 may allow the socket to accept connections, in which
case the length of the listen queue may be set to an implementation-defined
minimum value.


SCTP API simply chooses to ignore the "may".  It is still fully compliant
with POSIX in this regard.

-vlad
--

From: Alan Cox
Date: Wednesday, January 14, 2009 - 8:21 am

Linux chooses the interpretation that zero means one connection. Having a
single protocol variant do different things is not nice because  a lot
of code is designed to handle multiple address families and expect the
same non address handling behaviour.

So while it may be able to claim posix compliance, its not Linux
behaviour, and its relying on a specific interpretation of posix not
being used by the OS.

At the very least the current behaviour of the SCTP code is plain rude.
--

From: Vlad Yasevich
Date: Wednesday, January 14, 2009 - 8:53 am

I will submit a requirest to change this behavior in the spec, but I am not
terribly optimistic.  This has been specified for a very long time and there
might be applications taking advantage of the ability to shut listing off.

At this time, let's leave this as is.  A well written application should specify
the listen backlog anyway, otherwise it's depending on the "may" language in the
Posix spec and will not get consistent behavior across different systems.

-vlad
--

From: Alan Cox
Date: Wednesday, January 14, 2009 - 9:37 am

Seems fair enough.
--

Previous thread: [RFC PATCH] perf_counter: Add support for pinned and exclusive counter groups by Paul Mackerras on Wednesday, January 14, 2009 - 2:24 am. (7 messages)

Next thread: Re: lib/klist.c: bit 0 in pointer can't be used as flag by Jesper Nilsson on Wednesday, January 14, 2009 - 3:19 am. (8 messages)