Denys Vlasenko wrote:
> No, I not only removed SIGKILL, I also removed wait:
>
> /*kill(ts->shell_pid, SIGKILL);
> wait4(ts->shell_pid, NULL, 0, NULL);*/
>
> 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
> Ralf, can you review attached patch?
>
I updated to SVN 20262, which I assume is the same as the attached
patch. It works with my tests.
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.
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.
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.
Regards
Ralf Friedl
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
