Re: svn commit: r296109 - head/libexec/rlogind

2016-02-27 Thread Bruce Evans 

On Sat, 27 Feb 2016, Jilles Tjoelker wrote:


On Sat, Feb 27, 2016 at 09:48:05AM -0500, Pedro Giffuni wrote:

In the case of rlogind, note that the above limitation [FD_SETSIZE]
has disappeared by using poll(2).



I will add that FreeBSD has a native poll(2) implementation, it is
not a wrapper around select as some old postings would suggest.



I don't have any plans to do a massive shakeup against select(2), this
was some lower hanging fruit that was easy to change. For new code
kqueue(2) should be preferred.


The FD_SETSIZE can be a more important issue in library code which may
be called from applications that have many descriptors open already.

I don't agree with always using kqueue(2) for new code. Provided poll(2)
has the necessary functionality and the number of file descriptors is
low, using poll(2) tends to result in simpler code and better
performance. Also, poll(2) is more portable and avoids a failure mode
from the kqueues rlimit.


All the conversions to poll() seem to be buggy.  poll() gives better
detection of EOF, but you have to actually check for it or you get
different behaviour from select().  gdb in FreeBSD has been broken
for 10-15 years by misconversion to poll().  E.g.:

ttyv1:bde@besplex:~> echo "p 2+2" | gdb -q
(gdb) Hangup detected on fd 0
error detected on stdin

This is because it fails to check for POLLIN on stdin when it sees
POLLHUP on stdin.  It loses all the buffered input.  I tried more
input.  It loses all input for a buffer size of < 8K, then starts
seeing some.  The details depend on the kernel buffering.  PIPE_BUF
is only 512 but the kernel normally uses larger buffers than that.

kdump output for this:

   987 cat  CALL  write(0x1,0x806,0x1fff)
   987 cat  GIO   fd 1 wrote 4096 bytes
   "
p 2+2
p 2+2
...
p 2"
   987 cat  RET   write 8191/0x1fff
   987 cat  CALL  read(0x3,0x806,0x4000)
   987 cat  GIO   fd 3 read 0 bytes
   ""
   987 cat  RET   read 0
   987 cat  CALL  close(0x3)
   987 cat  RET   close 0
   987 cat  CALL  close(0x1)
   987 cat  RET   close 0
   987 cat  CALL  exit(0)
   986 sh   RET   wait4 987/0x3db
   986 sh   CALL  wait4(0x,0xbfbfe7c8,0x2,0)
   988 gdb  RET   poll 1
   988 gdb  CALL  write(0x1,0x830,0x18)
   988 gdb  GIO   fd 1 wrote 24 bytes
   "Hangup detected on fd 0
   "
   988 gdb  RET   write 24/0x18
   988 gdb  CALL  write(0x1,0x830,0x18)
   988 gdb  GIO   fd 1 wrote 24 bytes
   "error detected on stdin
   "
   988 gdb  RET   write 24/0x18
   988 gdb  CALL  sigprocmask(0x1,0,0xbfbfe33c)
   988 gdb  RET   sigprocmask 0
   988 gdb  CALL  exit(0)
   986 sh   RET   wait4 988/0x3dc
   986 sh   CALL  exit(0)

The changes have the opposite problem.  They check for POLLIN but not
POLLHUP.  This depends on the unportable and broken but possibly
permittied behaviour of poll() always setting POLLIN when it sets
POLLHUP.

select() has no way to report POLLHUP, so it converts POLLHUP to
input-ready.  poll() should do the following:
- for hangup with buffered input, as in the above pipe case, set
  POLLHUP to indicate the hangup and POLLIN to indicate the data
- for hangup with no buffered input, set POLLHUP to indicate the
  hangup and clear POLLIN to indicate no data
but the usual misimplementation does:
- always set POLLIN when setting POLLHUP.  This makes poll() behave
  more like select(), so that buggy conversions to poll() work, but
  it means that POLLIN cannot be trusted for determining if more
  than non-null data can be read.  This is always done for regular
  files IIRC.  For regular files, there is never any buffered input
  POLLHUP with POLLIN really means POLLHUP without POLLIN.
Another possible misimplementation that I have not observed is:
- delay setting POLLHUP until all buffered input has been read or
  discardered.  This would work OK for terminals since terminals
  are specified to discard input on hangup (but there are many races
  at low levels with the timing of the hangup and the input -- many
  drivers lose input much like gdb does -- they deliver the hangup
  to upper layers synchronously, but deliver buffered input later).
  However, for pipes the input is not discarded, so POLLHUP should
  not be delayed, so that applications can decide if they want to
  discard the input on hanhyp.

The poll regression tests check which version of the bugs are implemented
for pipes and sockets.  They only pass for some file types because the
bugs are implemented for others.  The buggy cases are system-dependent.

Due to the bugs, POLLIN together with POLLHUP can never be trusted.
Applications should use similar code to after select() to detect EOF:

switch (events & (POLLHUP | POLLIN)) {
case POLLHUP:
/* Non-buggy case. */
exit(0);
case POLLIN:
/* Normal input. */

Re: svn commit: r296109 - head/libexec/rlogind

2016-02-27 Thread Pedro Giffuni 



On 02/27/16 13:38, Jilles Tjoelker wrote:

On Sat, Feb 27, 2016 at 09:48:05AM -0500, Pedro Giffuni wrote:

In the case of rlogind, note that the above limitation [FD_SETSIZE]
has disappeared by using poll(2).



I will add that FreeBSD has a native poll(2) implementation, it is
not a wrapper around select as some old postings would suggest.



I don't have any plans to do a massive shakeup against select(2), this
was some lower hanging fruit that was easy to change. For new code
kqueue(2) should be preferred.


The FD_SETSIZE can be a more important issue in library code which may
be called from applications that have many descriptors open already.

I don't agree with always using kqueue(2) for new code. Provided poll(2)
has the necessary functionality and the number of file descriptors is
low, using poll(2) tends to result in simpler code and better
performance. Also, poll(2) is more portable and avoids a failure mode
from the kqueues rlimit.



Of course it pretty much depends on what you want to do. Yes, for 
something like talk we are fine with poll(). I also find poll()

more readable than the alternatives.

Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r296109 - head/libexec/rlogind

2016-02-27 Thread Jilles Tjoelker 
On Sat, Feb 27, 2016 at 09:48:05AM -0500, Pedro Giffuni wrote:
> In the case of rlogind, note that the above limitation [FD_SETSIZE]
> has disappeared by using poll(2).

> I will add that FreeBSD has a native poll(2) implementation, it is
> not a wrapper around select as some old postings would suggest.

> I don't have any plans to do a massive shakeup against select(2), this
> was some lower hanging fruit that was easy to change. For new code
> kqueue(2) should be preferred.

The FD_SETSIZE can be a more important issue in library code which may
be called from applications that have many descriptors open already.

I don't agree with always using kqueue(2) for new code. Provided poll(2)
has the necessary functionality and the number of file descriptors is
low, using poll(2) tends to result in simpler code and better
performance. Also, poll(2) is more portable and avoids a failure mode
from the kqueues rlimit.

-- 
Jilles Tjoelker
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r296109 - head/libexec/rlogind

2016-02-27 Thread Pedro Giffuni 

Hello;



On 02/27/16 08:08, Ronald Klop wrote:

Hi,

I'm not a FreeBSD commiter, but read these commits to stay informed
about what is happening.
This commit (and the one for talk) mentions _what_ is changed, but not
_why_ it is changed. I'm curious why this is better. Mind to share any
comments on this?




Sure. The difference is very small and it could even be argued if the
change is worth it at all. Both poll(2) and select(2) do the same and
have basically the same performance but poll(2) is a bit more scalable.


Regards,
Ronald.


On Fri, 26 Feb 2016 21:02:02 +0100, Pedro F. Giffuni 
wrote:


...

Modified: head/libexec/rlogind/rlogind.c
==

--- head/libexec/rlogind/rlogind.cFri Feb 26 19:49:04 2016
(r296108)
+++ head/libexec/rlogind/rlogind.cFri Feb 26 20:02:01 2016
(r296109)
@@ -76,6 +76,7 @@ __FBSDID("$FreeBSD$");
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -350,34 +351,27 @@ protocol(int f, int p)
 nfd = f + 1;
 else
 nfd = p + 1;
-if (nfd > FD_SETSIZE) {
-syslog(LOG_ERR, "select mask too small, increase FD_SETSIZE");
-fatal(f, "internal error (select mask too small)", 0);
-}



In the case of rlogind, note that the above limitation has disappeared
by using poll(2).

I will add that FreeBSD has a native poll(2) implementation, it is
not a wrapper around select as some old postings would suggest.

I don't have any plans to do a massive shakeup against select(2), this
was some lower hanging fruit that was easy to change. For new code
kqueue(2) should be preferred.

Pedro.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r296109 - head/libexec/rlogind

2016-02-27 Thread Ronald Klop 

Hi,

I'm not a FreeBSD commiter, but read these commits to stay informed about  
what is happening.
This commit (and the one for talk) mentions _what_ is changed, but not  
_why_ it is changed. I'm curious why this is better. Mind to share any  
comments on this?


Regards,
Ronald.


On Fri, 26 Feb 2016 21:02:02 +0100, Pedro F. Giffuni   
wrote:



Author: pfg
Date: Fri Feb 26 20:02:01 2016
New Revision: 296109
URL: https://svnweb.freebsd.org/changeset/base/296109

Log:
  rlogin(1): Replace select(2) with poll(2).
 Obtanied from: NetBSD (CVS Rev. 1.27 - 1.28)

Modified:
  head/libexec/rlogind/rlogind.c

Modified: head/libexec/rlogind/rlogind.c
==
--- head/libexec/rlogind/rlogind.c  Fri Feb 26 19:49:04 2016
(r296108)
+++ head/libexec/rlogind/rlogind.c  Fri Feb 26 20:02:01 2016
(r296109)
@@ -76,6 +76,7 @@ __FBSDID("$FreeBSD$");
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -350,34 +351,27 @@ protocol(int f, int p)
nfd = f + 1;
else
nfd = p + 1;
-   if (nfd > FD_SETSIZE) {
-   syslog(LOG_ERR, "select mask too small, increase FD_SETSIZE");
-   fatal(f, "internal error (select mask too small)", 0);
-   }
for (;;) {
-   fd_set ibits, obits, ebits, *omask;
+   struct pollfd set[2];
-   FD_ZERO();
-   FD_ZERO();
-   FD_ZERO();
-   omask = (fd_set *)NULL;
-   if (fcc) {
-   FD_SET(p, );
-   omask = 
-   } else
-   FD_SET(f, );
+   set[0].fd = p;
+   set[0].events = POLLPRI;
+   set[1].fd = f;
+   set[1].events = 0;
+   if (fcc)
+   set[0].events |= POLLOUT;
+   else
+   set[1].events |= POLLIN;
if (pcc >= 0) {
-   if (pcc) {
-   FD_SET(f, );
-   omask = 
-   } else
-   FD_SET(p, );
+   if (pcc)
+   set[1].events |= POLLOUT;
+   else
+   set[0].events |= POLLIN;
}
-   FD_SET(p, );
-   if ((n = select(nfd, , omask, , 0)) < 0) {
+   if ((n = poll(set, 2, INFTIM)) < 0) {
if (errno == EINTR)
continue;
-   fatal(f, "select", 1);
+   fatal(f, "poll", 1);
}
if (n == 0) {
/* shouldn't happen... */
@@ -385,18 +379,16 @@ protocol(int f, int p)
continue;
}
 #definepkcontrol(c)
((c)&(TIOCPKT_FLUSHWRITE|TIOCPKT_NOSTOP|TIOCPKT_DOSTOP))
-   if (FD_ISSET(p, )) {
+   if (set[0].revents & POLLPRI) {
cc = read(p, , 1);
if (cc == 1 && pkcontrol(cntl)) {
cntl |= oobdata[0];
send(f, , 1, MSG_OOB);
-   if (cntl & TIOCPKT_FLUSHWRITE) {
+   if (cntl & TIOCPKT_FLUSHWRITE)
pcc = 0;
-   FD_CLR(p, );
-   }
}
}
-   if (FD_ISSET(f, )) {
+   if (set[1].revents & POLLIN) {
fcc = read(f, fibuf, sizeof(fibuf));
if (fcc < 0 && errno == EWOULDBLOCK)
fcc = 0;
@@ -422,11 +414,10 @@ protocol(int f, int p)
goto top; /* n^2 */
}
}
-   FD_SET(p, );  /* try write */
}
}
-   if (FD_ISSET(p, ) && fcc > 0) {
+   if (set[0].revents & POLLOUT && fcc > 0) {
cc = write(p, fbp, fcc);
if (cc > 0) {
fcc -= cc;
@@ -434,7 +425,7 @@ protocol(int f, int p)
}
}
-   if (FD_ISSET(p, )) {
+   if (set[0].revents & POLLIN) {
pcc = read(p, pibuf, sizeof (pibuf));
pbp = pibuf;
if (pcc < 0 && errno == EWOULDBLOCK)
@@ -443,7 +434,6 @@ protocol(int f, int p)
break;
else if (pibuf[0] == 0) {
pbp++, pcc--;
-

svn commit: r296109 - head/libexec/rlogind

2016-02-26 Thread Pedro F. Giffuni 
Author: pfg
Date: Fri Feb 26 20:02:01 2016
New Revision: 296109
URL: https://svnweb.freebsd.org/changeset/base/296109

Log:
  rlogin(1): Replace select(2) with poll(2).
  
  Obtanied from:NetBSD (CVS Rev. 1.27 - 1.28)

Modified:
  head/libexec/rlogind/rlogind.c

Modified: head/libexec/rlogind/rlogind.c
==
--- head/libexec/rlogind/rlogind.c  Fri Feb 26 19:49:04 2016
(r296108)
+++ head/libexec/rlogind/rlogind.c  Fri Feb 26 20:02:01 2016
(r296109)
@@ -76,6 +76,7 @@ __FBSDID("$FreeBSD$");
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -350,34 +351,27 @@ protocol(int f, int p)
nfd = f + 1;
else
nfd = p + 1;
-   if (nfd > FD_SETSIZE) {
-   syslog(LOG_ERR, "select mask too small, increase FD_SETSIZE");
-   fatal(f, "internal error (select mask too small)", 0);
-   }
for (;;) {
-   fd_set ibits, obits, ebits, *omask;
+   struct pollfd set[2];
 
-   FD_ZERO();
-   FD_ZERO();
-   FD_ZERO();
-   omask = (fd_set *)NULL;
-   if (fcc) {
-   FD_SET(p, );
-   omask = 
-   } else
-   FD_SET(f, );
+   set[0].fd = p;
+   set[0].events = POLLPRI;
+   set[1].fd = f;
+   set[1].events = 0;
+   if (fcc)
+   set[0].events |= POLLOUT;
+   else
+   set[1].events |= POLLIN;
if (pcc >= 0) {
-   if (pcc) {
-   FD_SET(f, );
-   omask = 
-   } else
-   FD_SET(p, );
+   if (pcc)
+   set[1].events |= POLLOUT;
+   else
+   set[0].events |= POLLIN;
}
-   FD_SET(p, );
-   if ((n = select(nfd, , omask, , 0)) < 0) {
+   if ((n = poll(set, 2, INFTIM)) < 0) {
if (errno == EINTR)
continue;
-   fatal(f, "select", 1);
+   fatal(f, "poll", 1);
}
if (n == 0) {
/* shouldn't happen... */
@@ -385,18 +379,16 @@ protocol(int f, int p)
continue;
}
 #definepkcontrol(c)
((c)&(TIOCPKT_FLUSHWRITE|TIOCPKT_NOSTOP|TIOCPKT_DOSTOP))
-   if (FD_ISSET(p, )) {
+   if (set[0].revents & POLLPRI) {
cc = read(p, , 1);
if (cc == 1 && pkcontrol(cntl)) {
cntl |= oobdata[0];
send(f, , 1, MSG_OOB);
-   if (cntl & TIOCPKT_FLUSHWRITE) {
+   if (cntl & TIOCPKT_FLUSHWRITE)
pcc = 0;
-   FD_CLR(p, );
-   }
}
}
-   if (FD_ISSET(f, )) {
+   if (set[1].revents & POLLIN) {
fcc = read(f, fibuf, sizeof(fibuf));
if (fcc < 0 && errno == EWOULDBLOCK)
fcc = 0;
@@ -422,11 +414,10 @@ protocol(int f, int p)
goto top; /* n^2 */
}
}
-   FD_SET(p, );  /* try write */
}
}
 
-   if (FD_ISSET(p, ) && fcc > 0) {
+   if (set[0].revents & POLLOUT && fcc > 0) {
cc = write(p, fbp, fcc);
if (cc > 0) {
fcc -= cc;
@@ -434,7 +425,7 @@ protocol(int f, int p)
}
}
 
-   if (FD_ISSET(p, )) {
+   if (set[0].revents & POLLIN) {
pcc = read(p, pibuf, sizeof (pibuf));
pbp = pibuf;
if (pcc < 0 && errno == EWOULDBLOCK)
@@ -443,7 +434,6 @@ protocol(int f, int p)
break;
else if (pibuf[0] == 0) {
pbp++, pcc--;
-   FD_SET(f, );  /* try write */
} else {
if (pkcontrol(pibuf[0])) {
pibuf[0] |= oobdata[0];
@@ -452,18 +442,8 @@ protocol(int f, int p)
pcc = 0;
}
}
-   if ((FD_ISSET(f,