Notes |
(0002400)
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, |
| |
(0002406)
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] |
| |
(0002407)
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 |
| |
(0002408)
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. |
| |
(0002409)
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? |
| |
(0002410)
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. |
| |
(0002417)
iggarpe
05-30-07 05:26
|
Answers:
1) Yes, without "-h" slattach must sleep forever. That is what the original slattach does.
2) "-e" is useless. As far as I know, there is no way to keep an SLIP interface running withough keeping the serial device handle open. As soon as slattach exits the sl0 interface stops working.
This means that any useful usage of slattach implies running in the background keeping the serial device open. That is why I implemented multiple TTY support. In my system, I have up to 12 SLIP interfaces, and I'd rather have a single slattach process in the background doing nothing but sleeping (it doesn't even check for hangup in my case).
3) Yes, as it is now, multiple TTY support makes no sense if "-h" is used, since hangup on one TTY would terminate slattach closing all other TTYs. A proper implementation would just close this TTY and keep waiting until hangup on all interfaces, however, as you point out, this scenario will hardly happen (one would usually want to call again on the interface which hang up).
Again, multiple TTY support is ONLY useful in a scenario in which you just want to setup a bunch of SLIP interfaces and go. Linux should allow a way to do this without forcing to have a process sleeping while keeping the devices open. |
| |
(0002418)
bernhardf
05-30-07 05:30
|
Please update the patch to add a config option for handling multiple ttys then. |
| |
(0002488)
iggarpe
06-19-07 04:50
|
It turns out making multiple tty support optional resulted in a #if/#else mess if you really want to reduce to the maximum the fat of the single tty version.
Since in addition, this is definitely a non-standard feature (as compared to the net-tools slattach), it is simply not worth the trouble.
I just removed the multiple tty support.
I hope the slattach applet is now fit enough as to be included in the main busybox trun. |
| |
(0002491)
bernhardf
06-20-07 03:14
|
$ patch --dry-run -p0 -i slattach-6.diff
patching file ./include/applets.h
Hunk 0000001 FAILED at 270.
1 out of 1 hunk FAILED -- saving rejects to file ./include/applets.h.rej
patching file ./include/usage.h
Hunk 0000001 succeeded at 2991 (offset 103 lines).
patching file ./networking/Config.in
Hunk 0000001 succeeded at 548 (offset 25 lines).
patching file ./networking/Kbuild
Hunk 0000001 succeeded at 27 with fuzz 2 (offset -4 lines).
patching file ./networking/slattach.c
Please rediff against trunk. thanks, |
| |
(0002492)
vda
06-20-07 08:24
|
Applied as rev 18873. Thanks. (I had to reformat usage message and to rediff to svn, though. Also nuked static variables while at it) |
| |
busybox-1.5.1-slattach.patch [^] (9,986 bytes) 05-24-07 05:28
