new applet: script

[Denys Vlasenko]

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).

Can you also do a "struct globals" trick and thus prevent
globals from eating data/bss? (See e.g. httpd.c for an example).


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.


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?

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?


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.
        static char *cflg, mode[] = "w", shell_arg[] = "-i";
        static int fflg;
None of these need to be static.


while ((ch = getopt(argc, argv, "ac:fq")) != -1)...

Please use getopt32 instead.


        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().


        if ((child = fork()) < 0) {
                fail("fork");
You can use bb_perror_msg_and_die("fork").
        }
        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).
        }
        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?


                        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.


                        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.
--
vda