Re: [eboard-devel] [mdz@debian.org: Bug#116722: eboard: More flexible auto-login]

[Matt Z. <md...@de...>]

On Tue, Oct 23, 2001 at 02:53:55AM -0400, Felipe Bergo wrote:

> On Mon, 22 Oct 2001, Daniel Burrows wrote:
> 
> > Of course, "perl" seems to be hardcoded in script.cc, but I see no
> > reason why it couldn't simply execute a script named by the user (which
> > could specify its own interpreter).
> 
> The original reason is: a long long time ago, I tried "/bin/sh -c
> script-path" or something similar which I can't recall and for some odd
> reason, with Perl, I'd get a bash child process and perl under it, but
> perl would change its PGID, so eboard would fail to kill the script when
> the user clicked the Kill button.
>
> Now, much after that, networking code (which has process spawning with 
> pipes) has changed (mostly to fix issues with engines), and after getting 
> this piece of email I tried again the /bin/sh -c thing and it seems it is 
> working without problems. The only change was in script.cc:
> 
> -  child=new PipeConnection("perl",fp,0,0,0);
> +  child=new PipeConnection("/bin/sh","-c",fp,0,0);
> 
> (fp is the script's path, if you're wondering, the implementation of 
> PipeConnection is in network.cc)

The right thing to do is to execute the program or script directly, with
no intervening interpreter or shell.  The shell will only be necessary
if you want to expand shell metacharacters in the command.  Looking at
PipeConnection, it seems like:

child = new PipeConnection(fp);

would be the correct incantation, resulting in

execvp(fp, {fp, 0, 0, 0});

Also, rather than forcing the script path to be
$HOME/.eboard/scripts/%s, it might make more sense to just add that
directory to $PATH, and let it be found by execvp.  This way, scripts
could be shared in the normal system path as well.

Now that I'm looking, I feel a little nervous about this code as well:

  p=getenv("HOME");
  if (!p) p="/tmp";

If for any reason the user's HOME environment variable isn't set,
who-knows-what could be executed out of /tmp/.eboard/....  The above
idea would eliminate this worry as well, as you could simply leave PATH
as-is if you can't determine the user's home directory.  If you want to
try harder, of course, you can do a getpwuid and get the home directory
that way before giving up.

I also have some suggestions about PipeConnection::open:

- Be sure to check for an error return from fork() and report it to the
  user; without that information, it can be difficult for a user to
  debug problems:

  pid = fork();
  if (pid == -1) {
    ...error, show strerror(errno)...
  } else if (pid == 0) {
    ...child...
  } else {
    ...parent...
  }

> As for the expect Starting line, today was my first try at expect, but for 
> some reason eboard doesn't seem to get the last "send" if the script 
> terminates right after it.

- Be sure to close the other side of the pipe after forking:

  ...fork...
  parent: close(n2h[0]); close(h2n[1]);
  child: close(n2h[1]); close(h2n[0]);

  This may have something to do with not getting all of the output from
  the child.

- When signalling the child process to shut down, SIGHUP or SIGTERM is
  more appropriate than SIGKILL.  The child process may have its own
  cleanup to do.

> Also there's a problem that eboard uses the second # comment in the
> file to get a script description to show in the scripts dialog. With
> languages that don't accept # for comments, this can get tricky.

Instead, maybe it should run the script with a special parameter telling
it to output a description (--eboard-description?).

Lest I seem like I have only criticism, I'd like to say that reading
through the eboard code was a lot less harrowing than, say, xboard's.
Keep up the good work.

-- 
 - mdz