[BusyBox 0001370]: slattach applet
bugs at busybox.net
bugs at busybox.net
Sun May 27 22:02:57 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-27-2007 15:02 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
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
======================================================================
More information about the busybox-cvs
mailing list