Ash + telnetd: telnet client hangs after exit

[Denys Vlasenko]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: z.diff
Type: text/x-diff
Size: 4385 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20071016/5236d81e/attachment-0002.bin