new applet: script
Pascal Bellard
pascal.bellard at ads-lu.com
Mon Feb 25 18:08:59 UTC 2008
Here is the updated patch.
-pascal
> On Sunday 24 February 2008 00:56, Pascal Bellard wrote:
>> Hello,
>>
>> Please find attached a patch to add script feature in busybox.
>> May be usefull to log boot output, report bugs...
>
> static FILE *fscript;
>
> You seem to ever use it only for fwrite(), not fprintf.
> Just use low-level fd-based I/O then (open, not fopen).
Fixed
>
> Can you also do a "struct globals" trick and thus prevent
> globals from eating data/bss? (See e.g. httpd.c for an example).
Fixed.
>
>
> static void done(void)
> {
> if (subchild) {
> fclose(fscript);
> close(pty);
> }
> else {
> tcsetattr(0, TCSAFLUSH, &tt);
> if (qflg == 0) printf("Script done, file is %s\n", fname);
> }
> exit(0);
> }
>
> exit() will close files. Don't bother doing it yourself.
Fixed.
>
>
> static void finish(int sig)
> {
> union wait status;
> int pid, die = 0;
>
> (void) sig;
> while ((pid = wait3((int *)&status, WNOHANG, 0)) > 0) {
> if (pid == child) {
> die = 1;
> }
> }
> if (die) {
> done();
> }
> }
>
> How about less cryptic one like this?
Maybe I'm wrong, but I want to catch all pending signals.
>
> static void sigchld_handler(int sig)
> {
> pid_t pid;
> while ((pid = wait_any_nohang(&sig) > 0)
> if (pid == child)
> done(); /* exits */
> }
>
>
> static void fail(const char *s)
> {
> bb_perror_msg(s);
> kill(0, SIGTERM);
> done();
> }
>
> I do not understand why you send TERM to yourself.
> Especially that you don't have a handler for it.
> Comment?
It wakes up the parent with SIGCHLD
>
>
> int script_main(int argc, char *argv[]) MAIN_EXTERNALLY_VISIBLE;
> int script_main(int argc, char *argv[])
> {
> int count, ch;
> struct termios rtt;
> const char *shell;
> struct winsize win;
> char line[32], buf[BUFSIZ];
> BUFSIZ on stack is too big.
now 256 ?
> static char *cflg, mode[] = "w", shell_arg[] = "-i";
> static int fflg;
> None of these need to be static.
Fixed.
>
>
> while ((ch = getopt(argc, argv, "ac:fq")) != -1)...
>
> Please use getopt32 instead.
Fixed
>
>
> if ((shell = getenv("SHELL")) == NULL) {
> shell = _PATH_BSHELL;
> }
> if ((pty = getpty(line,sizeof(line))) < 0) {
> fail("Out of pty's");
> }
>
> For better readability, pull assignments out of if().
Fixed.
>
>
> if ((child = fork()) < 0) {
> fail("fork");
> You can use bb_perror_msg_and_die("fork").
Fixed.
> }
> if (child) { /* parent */
> fclose(fscript);
> while ((count = read(0, buf, sizeof(buf))) > 0) {
> write(pty, buf, count);
> }
> Use bb_copyfd_eof(src_fd, dst_fd).
Fixed.
> }
> else { /* child */
> if ((subchild = child = fork()) < 0) {
> fail("fork");
> }
> if (child) { /* parent */
>
> Can you add a comment why two levels of forks are needed,
> and how data is piped through pty fds?
Fixed.
parent: link mainshell stdin to pty input
child1: link pty output to mainshell stdout and file
child2: link subshell input, output, error to pty
>
>
> pty = xopen(line, O_RDWR);
> tcsetattr(pty, TCSAFLUSH, &tt);
> ioctl(pty, TIOCSWINSZ, (char *)&win);
> setsid();
> ioctl(pty, TIOCSCTTY, 0);
> dup2(pty, 0);
> dup2(pty, 1);
> dup2(pty, 2);
> close(pty);
>
> About this close() - are you sure pty is > 2? IOW: use
> "xmove_fd(pty, 0); dup2(0, 1); dup2(0, 2);" instead.
Fixed. Safer and shorter !
>
>
> execl(shell, strrchr(shell, '/') + 1, shell_arg,
> cflg, 0);
>
> My gcc said "util-linux/script.c:147: warning: missing sentinel in
> function call"
> (first time ever I see such a meesage. gcc is 4.2.1). s/0/NULL/ helped.
Fixed.
> --
> vda
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-20080224-APPLET-SCRIPT.u
Type: application/octet-stream
Size: 9731 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20080225/c16d0abd/attachment-0002.obj
More information about the busybox
mailing list