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