On Thu, Dec 06, 2018 at 12:43:02PM +0100, Rick van Rein wrote:
> Besides output to a file, this patch introduces commands for
> interactive streaming, such as over a network connection.
Nice idea ;-)
> A default "scriptstream" command may be locally installed with
> all the bells and whistles that make sense locally.
[...]
> #include <stdio.h>
> @@ -97,12 +100,16 @@ UL_DEBUG_DEFINE_MASKNAMES(script) =
> UL_DEBUG_EMPTY_MASKNAMES;
> #endif
>
> #define DEFAULT_TYPESCRIPT_FILENAME "typescript"
> +#define DEFAULT_TYPESCRIPT_COMMAND "scriptstream"
Do we really need the default command? ;-)
I think the example with nc(1) in the man page is good enough. We do
not need hardcode any path/name in the code.
> struct script_control {
> char *shell; /* shell to be executed */
> char *command; /* command to be executed */
> char *fname; /* output file path */
> FILE *typescriptfp; /* output file pointer */
> + int stream; /* output to streaming command */
It would be better to use one bit for this boolean,
unsigned int
stream :1
see and of the struct script_control where are already another
options.
[...]
> +static void wait_for_stream_cmd(struct script_control *ctl, int wait)
> +{
> + int options = wait ? 0 : WNOHANG;
> + if (ctl->stream && ctl->typescriptfp) {
> + DBG(MISC, ul_debug("closing stream subcommand"));
> + fclose (ctl->typescriptfp);
> + ctl->typescriptfp = NULL;
> + kill(ctl->streampid, SIGTERM);
> + }
> +
> + DBG(MISC, ul_debug("waiting for stream subcommand"));
> +
> + waitpid(ctl->streampid, &ctl->streamstatus, options);
> }
>
> static void write_output(struct script_control *ctl, char *obuf,
> @@ -483,6 +505,7 @@ static void handle_signal(struct script_control
> *ctl, int fd)
> || info.ssi_code == CLD_KILLED
> || info.ssi_code == CLD_DUMPED) {
> wait_for_child(ctl, 0);
> + wait_for_stream_cmd(ctl, 0);
if (ctl->stream)
wait_for_stream_cmd(ctl, 0);
I guess it would be more robust, than call wait_for_stream_cmd()
in all situations.
> ctl->poll_timeout = 10;
>
> /* In case of ssi_code is CLD_TRAPPED, CLD_STOPPED, or CLD_CONTINUED */
> @@ -519,6 +542,7 @@ static void handle_signal(struct script_control
> *ctl, int fd)
> static void do_io(struct script_control *ctl)
> {
> int ret, eof = 0;
> + int cmdio[2];
It's detail, but stream_pipe[] wold be more readable :-)
> time_t tvec = script_time((time_t *)NULL);
> enum {
> POLLFD_SIGNAL = 0,
> @@ -533,9 +557,33 @@ static void do_io(struct script_control *ctl)
> };
>
>
> - if ((ctl->typescriptfp =
> - fopen(ctl->fname, ctl->append ? "a" UL_CLOEXECSTR : "w"
> UL_CLOEXECSTR)) == NULL) {
> -
> + if (ctl->stream) {
> + if (pipe(cmdio) == -1) {
> + warn(_("pipe failed"));
> + fail(ctl);
> + exit(1);
> + }
> + ctl->streampid = fork();
> + switch (ctl->streampid) {
> + case -1: /* error */
> + warn(_("fork failed"));
> + fail(ctl);
> + exit(1);
> + case 0: /* child */
> + close(cmdio[1]);
> + dup2(cmdio[0],0);
> + execvp(ctl->streamcmd[0], ctl->streamcmd);
> + fprintf(stderr,_("streaming command failed: %s\r\n"),strerror(errno));
> + exit(1);
err(1, _("streaming command failed"));
err() seems better than fprintf() + strerror() + exit();
[...]
> + /* streaming may be ordered with option --stream or with argc > 1 */
> + if (argc == 0) {
> + if (ctl.stream) {
> + ctl.minicmd[0] = DEFAULT_TYPESCRIPT_COMMAND;
> + ctl.minicmd[1] = NULL;
> + ctl.streamcmd = &ctl.minicmd[0];
> + } else {
> + /* No default implementation to make this pluggable */
> + ctl.fname = DEFAULT_TYPESCRIPT_FILENAME;
> + die_if_link(&ctl);
> + }
> + } else if (argc == 1) {
> + if (ctl.stream) {
> + ctl.minicmd[0] = argv[0];
> + ctl.minicmd[1] = NULL;
> + ctl.streamcmd = &ctl.minicmd[0];
> + } else {
> + ctl.fname = argv[0];
> + }
> + } else {
> + /* implicit streaming for more than 1 command argument */
> + ctl.streamcmd = argv;
> + ctl.stream = 1;
> + ctl.flush = 1;
> }
Frankly, all the command-line semantic seems complicated and a little
bit like over-engineering. It would be nice to keep it simple and
stupid :)
script
script <filename>
script --stream <command> [arg ...]
The first two ways write to "typescript" or to <filename>. The last
writes to <command> [arg ...].
The options --stream should be no_argument (as you already have) and
<command> [arg ...] is all argv[] after argv += optind.
All you need is
struct script_control {
char **stream_argv;
...
};
main() {
argv += optind;
if (argc > 0) {
if (ctl.stream)
ctl.stream_argv = &argv[0];
else
ctl.fname = argv[0];
}
if (ctl.stream && !ctl.stream_argv)
err(EXIT_FAILURE, _("streaming command not specified"));
}
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
