[BusyBox 0001370]: slattach applet

bugs at busybox.net bugs at busybox.net
Tue May 29 01:42:40 UTC 2007


A NOTE has been added to this issue. 
====================================================================== 
http://busybox.net/bugs/view.php?id=1370 
====================================================================== 
Reported By:                iggarpe
Assigned To:                BusyBox
====================================================================== 
Project:                    BusyBox
Issue ID:                   1370
Category:                   Documentation
Reproducibility:            always
Severity:                   feature
Priority:                   normal
Status:                     assigned
====================================================================== 
Date Submitted:             05-24-2007 05:28 PDT
Last Modified:              05-28-2007 18:42 PDT
====================================================================== 
Summary:                    slattach applet
Description: 
The summary says it all. Fully tested and working fine.

There's a very small room for improvement in the command line parsin code,
in which I didn't use the libbb facilities because I'm not familiarized
with them and could not find documentation on libbb.


====================================================================== 

---------------------------------------------------------------------- 
 bernhardf - 05-24-07 10:05  
---------------------------------------------------------------------- 
1) see
http://busybox.net/lists/busybox/2007-January/025845.html

2) Instead of parsing _param yourself, use index_in_str_array.

3) use getopt32() instead of the hand-written argument parsing.

4) sleep_and_die nowaday is xfunc_die() , IIRC

5) variable "local" needs to be a bool

6) given that watch, quit, raw, flow also should be bools, check if using
one smalluint with appropriate masks is smaller than all bools (it should
be smaller than many bools).

7) about all includes are superfluous. You have to include busybox.h
first, else this applet will fail to compile miserable on a number of
systems.

8) Due to the points raised above, it's no wonder that this applet is
tremendously bloated:
   1644       0       8    1652     674 slattach.o
1.5 Kilobyte! This is inacceptable.

This functionality has to fit in well below 1k, if you ask me.

HTH, 

---------------------------------------------------------------------- 
 vda - 05-27-07 14:51  
---------------------------------------------------------------------- 
I attached a debloated version.
I would like to ask original bug submitter to review and test it.
I also have several questions:

1. Is it correct that restore_state_and_close() will bail out on any
error, not trying to restore remaining ttys?

2. Here:
        baud_code = tty_value_to_baud(baud);
        if (baud_code < 0)
                bb_error_msg_and_die("invalid baud rate %i", baud);
        ...
        for (...) {...
                if (baud_code >= 0) {
                        cfsetispeed(&state, baud_code);
                        cfsetospeed(&state, baud_code);
                }
        }
   we have two problems:

   (a) we use baud unconditionally, should we do it only if -s BAUD is
given?
       Otherwise we basically will fail if there is no -s. Is this
intended?

   (b) 'if (baud_code >= 0)' is always true.

[these problems are present in original code also IIUC] 

---------------------------------------------------------------------- 
 vda - 05-27-07 15:02  
---------------------------------------------------------------------- 
Actually, I see a buglet in second attachment + more trimming
opportunities.
So I will attach third version now. size:
# size slattach.o
   text    data     bss     dec     hex filename
   1278       0       4    1282     502 slattach.o 

---------------------------------------------------------------------- 
 iggarpe - 05-28-07 03:56  
---------------------------------------------------------------------- 
Took your v3 code and did small modifications:

1. As you point out, restore_state_and_close should actually try to
restore all states despite of errors. Corrected in slattach_v4.c.

2. The baud rate should only be set if option "-s" is present. In the
original code baud_rate would stay zero if the option wasn't found, thus
yielding the expected behaviour. Fixed in slattach_v4.c.

I've split the setup loop (the one which walks the linked list saving
state and setting new state) in two loops. The first loop just saves the
state of all ttys, and the second loop sets the new state. This way, the
save_state code does not need to clean up if something fails because no
tty has been modified yet.

Of course this adds a bit of bloat. An alternative way to achieve this is
to *
ALWAYS* exit via restore_state_and_exit, however, this woule require some
sort of flag in the tty structure telling if the tty state has already
been set, thus this flag can be checked by restore_state_and_exit.

BTW, the original slattach DOES NOT SUPPORT MULTIPLE TTYs. Given that the
slattach process stays running just to keep the device open, this is a
huge waste of resources in systems with lots of slip interfaces and  small
memory footprint (a damn lot of processes sitting there on their butts),
and this is why I originally implemented multiple tty support. I mean that
this feature could be removed to trim the code size. 

---------------------------------------------------------------------- 
 vda - 05-28-07 17:17  
---------------------------------------------------------------------- 
/* Watch line for hangup */
        for (;;) {
                if (opt & OPT_h_watch)
                        for (t = tty_head; t != NULL; t = t->next)
                                if (ioctl(t->handle, TIOCMGET, &i) < 0 ||
!(i & TIOCM_CAR))
                                        goto no_carrier;
                sleep(15);
        }

 no_carrier:


Question: if no -h is given, it will simply sleep forever. Does this match
"standard" slattach behaviour? 

---------------------------------------------------------------------- 
 vda - 05-28-07 18:42  
---------------------------------------------------------------------- 
Attached v5. Removed linked list in favor of simple vector, nuked ->next
and a few 'handle < 0' tests that are never true.

The biggest question is at the end of the file in the comment:

// Actually, "many ttys" feature seems to be useless in practice.
// * If you run slattach with -e, you may as well run many slattach'es.
// * Otherwise: what will happen whan just one tty line hangs up
//   while 99 other lines continue to run ok?
//   Leaving it hung up seems to be useless, in real life you
//   want to redial or otherwise reinit and restart communications.
//   How to do it using our still-running slattach instance?
//   With -c, we run a command, and then what? Line is still hung up.
//
//   Please describe real-world scenario how it is supposed to work.
//   Maybe it should eventually be in help text.

More comments are in the source. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
05-24-07 05:28  iggarpe        New Issue                                    
05-24-07 05:28  iggarpe        Status                   new => assigned     
05-24-07 05:28  iggarpe        Assigned To               => BusyBox         
05-24-07 05:28  iggarpe        File Added: busybox-1.5.1-slattach.patch         
          
05-24-07 10:05  bernhardf      Note Added: 0002400                          
05-27-07 14:42  vda            File Added: slattach.c                       
05-27-07 14:51  vda            Note Added: 0002406                          
05-27-07 15:02  vda            Note Added: 0002407                          
05-27-07 15:03  vda            File Added: slattach_v3.c                    
05-28-07 03:56  iggarpe        Note Added: 0002408                          
05-28-07 03:56  iggarpe        File Added: slattach_v4.c                    
05-28-07 17:17  vda            Note Added: 0002409                          
05-28-07 18:40  vda            File Added: slattach_v5.c                    
05-28-07 18:42  vda            Note Added: 0002410                          
======================================================================




More information about the busybox-cvs mailing list