On Tuesday 16 October 2007 11:24, Ralf Friedl wrote:
> > I don't wait for anything now. I tear down connections as soon
> > as there is a problem (or EOF) reading/writing to the pty.
> >
> I didn't look careful enough.
> May I suggest using #if 0 ? That makes it easier to read, and it also
> works with embedded comments.
>
> #if 0
> kill(ts->shell_pid, SIGKILL); /* Works even with comments */
> wait4(ts->shell_pid, NULL, 0, NULL);
> #endif
ok
> As Busybox is about reduced code size, also some unrelated remarks:
>
> In free_session():
> /* error if ts->sockfd_read == ts->sockfd_write. So what? ;) */
> - close(ts->sockfd_write);
> ...
> - if (maxfd < ts->sockfd_read)
> - maxfd = ts->sockfd_read;
> if (maxfd < ts->sockfd_write)
> maxfd = ts->sockfd_write;
> Actually, ts->sockfd_read == ts->sockfd_write, unless telnetd is called
> from inetd, in which case free_session is not called at all, so that
> close is not necessary. The same for testing maxfd against both
> sockfd_read and sockfd_write.
>
> In make_new_session():
> - if (sock_r > maxfd) maxfd = sock_r;
> ts->sockfd_read = sock_r;
> ndelay_on(sock_r);
> Similar to the previous, either ts->sockfd_read == ts->sockfd_write, or
> in the case called by inetd, ts->sockfd_read < ts->sockfd_write
> (ts->sockfd_read == 0, ts->sockfd_write == 1).
> If you apply this, it would be useful to add comments explaining the
> reasons for this.
ok
> In telnetd_main():
> /* Ignore trailing NUL if it is there */
> if (!TS_BUF1[ts->rdidx1 + count - 1]) {
> - if (!--count)
> - goto skip3;
> + --count;
> }
> ts->size1 += count;
> ts->rdidx1 += count;
> if (ts->rdidx1 >= BUFSIZE) /* actually == BUFSIZE */
> ts->rdidx1 = 0;
> }
> skip3:
> If (count == 0), the execution of the following lines has no effect. It
> takes a little longer, but at least my telnet client doesn't send
> trailing NULs most of the time. The question is code size or execution time.
ok
> In free_session() and other places, there are loops like this:
> /* Scan all sessions and find new maxfd */
> ts = sessions;
> while (ts) {
> ...
> ts = ts->next;
> }
> This can be written as:
> /* Scan all sessions and find new maxfd */
> for (ts = sessions; ts; ts = ts->next) {
> ...
> }
> The generated code should be the same, but the loop control is all in
> one line. This is mainly a matter of preferences. I consider the one
> line version easier to read.
>
> It is especially useful in cases like the loop in telnetd_main, where it
> wouldn't be necessary to write "ts = next;" before each "continue;" I
> don't know whether the compiler is clever enough to detect the common
> "ts = next; continue;", so in that case it may also save a few instructions.
I will also group free_session.
function old new delta
telnetd_main 1355 1350 -5
make_new_session 532 521 -11
free_session 118 101 -17
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-33) Total: -33 bytes
text data bss dec hex filename
676328 2538 12104 690970 a8b1a busybox_old
676295 2538 12104 690937 a8af9 busybox_unstripped
Please review attached (or see current svn).
--
vda
Index: networking/telnetd.c
===================================================================
--- networking/telnetd.c (revision 20263)
+++ networking/telnetd.c (working copy)
@@ -203,7 +203,7 @@
static struct tsession *
make_new_session(
- USE_FEATURE_TELNETD_STANDALONE(int sock_r, int sock_w)
+ USE_FEATURE_TELNETD_STANDALONE(int sock)
SKIP_FEATURE_TELNETD_STANDALONE(void)
) {
const char *login_argv[2];
@@ -221,19 +221,22 @@
bb_error_msg("can't create pty");
return NULL;
}
- if (fd > maxfd) maxfd = fd;
+ if (fd > maxfd)
+ maxfd = fd;
ts->ptyfd = fd;
ndelay_on(fd);
#if ENABLE_FEATURE_TELNETD_STANDALONE
- if (sock_w > maxfd) maxfd = sock_w;
- ts->sockfd_write = sock_w;
- ndelay_on(sock_w);
- if (sock_r > maxfd) maxfd = sock_r;
- ts->sockfd_read = sock_r;
- ndelay_on(sock_r);
+ ts->sockfd_read = sock;
+ ndelay_on(sock);
+ if (!sock) /* We are called with fd 0 - we are in inetd mode */
+ sock++;
+ ts->sockfd_write = sock;
+ ndelay_on(sock);
+ if (sock > maxfd)
+ maxfd = sock;
#else
+ /* ts->sockfd_read = 0; - done by xzalloc */
ts->sockfd_write = 1;
- /* ts->sockfd_read = 0; - done by xzalloc */
ndelay_on(0);
ndelay_on(1);
#endif
@@ -326,27 +329,33 @@
t->next = ts->next;
}
+#if 0
/* It was said that "normal" telnetd just closes ptyfd,
* doesn't send SIGKILL. When we close ptyfd,
* kernel sends SIGHUP to processes having slave side opened. */
- /*kill(ts->shell_pid, SIGKILL);
- wait4(ts->shell_pid, NULL, 0, NULL);*/
+ kill(ts->shell_pid, SIGKILL);
+ wait4(ts->shell_pid, NULL, 0, NULL);
+#endif
close(ts->ptyfd);
close(ts->sockfd_read);
- /* error if ts->sockfd_read == ts->sockfd_write. So what? ;) */
- close(ts->sockfd_write);
+ /* We do not need to close(ts->sockfd_write), it's the same
+ * as sockfd_read unless we are in inetd mode. But in inetd mode
+ * we do not free_session(), ever */
free(ts);
/* Scan all sessions and find new maxfd */
+ maxfd = 0;
ts = sessions;
- maxfd = 0;
while (ts) {
if (maxfd < ts->ptyfd)
maxfd = ts->ptyfd;
if (maxfd < ts->sockfd_read)
maxfd = ts->sockfd_read;
+#if 0
+ /* Again, sockfd_write == sockfd_read here */
if (maxfd < ts->sockfd_write)
maxfd = ts->sockfd_write;
+#endif
ts = ts->next;
}
}
@@ -434,7 +443,7 @@
#if ENABLE_FEATURE_TELNETD_STANDALONE
if (IS_INETD) {
- sessions = make_new_session(0, 1);
+ sessions = make_new_session(0);
if (!sessions) /* pty opening or vfork problem, exit */
return 1; /* make_new_session prints error message */
} else {
@@ -519,7 +528,7 @@
if (fd < 0)
goto again;
/* Create a new session and link it into our active list */
- new_ts = make_new_session(fd, fd);
+ new_ts = make_new_session(fd);
if (new_ts) {
new_ts->next = sessions;
sessions = new_ts;
@@ -545,9 +554,7 @@
goto skip1;
if (IS_INETD)
return 0;
- free_session(ts);
- ts = next;
- continue;
+ goto kill_session;
}
ts->size1 -= count;
ts->wridx1 += count;
@@ -564,9 +571,7 @@
goto skip2;
if (IS_INETD)
return 0;
- free_session(ts);
- ts = next;
- continue;
+ goto kill_session;
}
ts->size2 -= count;
ts->wridx2 += count;
@@ -577,7 +582,7 @@
/* Should not be needed, but... remove_iacs is actually buggy
* (it cannot process iacs which wrap around buffer's end)!
* Since properly fixing it requires writing bigger code,
- * we will rely instead on this code making it virtually impossible
+ * we rely instead on this code making it virtually impossible
* to have wrapped iac (people don't type at 2k/second).
* It also allows for bigger reads in common case. */
if (ts->size1 == 0) {
@@ -598,14 +603,11 @@
goto skip3;
if (IS_INETD)
return 0;
- free_session(ts);
- ts = next;
- continue;
+ goto kill_session;
}
/* Ignore trailing NUL if it is there */
if (!TS_BUF1[ts->rdidx1 + count - 1]) {
- if (!--count)
- goto skip3;
+ --count;
}
ts->size1 += count;
ts->rdidx1 += count;
@@ -622,9 +624,7 @@
goto skip4;
if (IS_INETD)
return 0;
- free_session(ts);
- ts = next;
- continue;
+ goto kill_session;
}
ts->size2 += count;
ts->rdidx2 += count;
@@ -633,6 +633,11 @@
}
skip4:
ts = next;
+ continue;
+ kill_session:
+ free_session(ts);
+ ts = next;
}
+
goto again;
}
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
