[m-rev.] for review: improve `mdb --window'

[Fergus Henderson]

On 29-Jan-2002, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> +++ configure.in	28 Jan 2002 15:50:55 -0000
... HAVE_*

Hmm... I think we should really be using `MR_' prefixes for those.
I guess that is an existing problem, but it would be nice to
avoid exacerbating it.

Oh, never mind, I guess that issue can wait for a separate change.

> +++ runtime/mercury_trace_base.c	28 Jan 2002 08:09:43 -0000
> +#if defined(HAVE_KILL) && defined(HAVE_WAIT) && defined(SIGTERM)
> +	if (MR_have_mdb_window) {
> +		int status;
> +		status = kill(MR_mdb_window_pid, SIGTERM);
> +		if (status != -1) {
> +			do {
> +				status = wait(NULL);
> +				if (status == -1 && errno != EINTR) {
> +					break;
> +				}
> +			} while (status != MR_mdb_window_pid);

Please add a comment here explaining what this code is supposed to do.

> Index: trace/mercury_trace_internal.c
...
> +		if (MR_mdb_in_window) {
...
> +		}
> +
> +		if (! MR_mdb_in_window) {
...
> +		}

I recommend writing that as

	if (MR_mdb_in_window) {
		...
	} else /* ! MR_mdb_in_window */ {
		...
	}

> +static bool MR_got_alarm = FALSE;

That should be `volatile sig_atomic_t' rather than `bool'.

> +static bool
> +MR_trace_internal_create_mdb_window(void)
> +{
> +	/*
> +	** XXX Add support for MS Windows.
> +	*/
> +#if defined(HAVE_OPEN) && defined(HAVE_FDOPEN) && defined(HAVE_CLOSE) && \
> +	defined(HAVE_DUP) && defined(HAVE_DUP2) && defined(HAVE_FORK) && \
> +	defined(HAVE_EXECLP)
> +
> +	int master_fd = -1;
> +	int slave_fd = -1;
> +	char *slave_name;
> +	pid_t child_pid;
> +#if defined(HAVE_TERMIOS_H) && defined(HAVE_TCGETATTR) && \
> +		defined(HAVE_TCSETATTR)
> +	struct termios termio;
> +#endif
> +
> +	/*
> +	** XXX This code to find and open a pseudo-terminal is nowhere
> +	** near as portable as I would like, but given the huge variety
> +	** of methods for allocating pseudo-terminals it will have to do.
> +	** Most systems seem to be standardising on this method (from UNIX98).
> +	** See the xterm or expect source for a more complete version
> +	** (it's a bit too entwined in the rest of the code to just lift
> +	** it out and use it here).
> +	*/

It might be better to put that comment at the start of the function body.

> +#if defined(HAVE_DEV_PTMX) && defined(HAVE_GRANTPT) && \
> +		defined(HAVE_UNLOCKPT) && defined(HAVE_PTSNAME)
> +	master_fd = open("/dev/ptmx", O_RDWR);

I think the use of `O_RDWR' here needs to be protected by an appropriate
#ifdef.

> +	if (master_fd == -1 || grantpt(master_fd) == -1
> +			|| unlockpt(master_fd) == -1) {
> +		close(master_fd);
> +		perror("error opening master pseudo-terminal for mdb window");

All error message output should go through either MR_warning()
or MR_fatal_error(), so you should use `MR_warning("...: %s", strerror(errno))'
rather than `perror("...")' here.

Likewise for all other calls to perror().
It may be worth defining MR_perror().

The `{' should be on a line of its own.

> +#else
> +	fprintf(stderr,
> +		"Sorry, `mdb --window' not supported on this platform.\n");

Likewise, this should go via MR_warning().

Or alternatively, if the idea is to deliberately hide the fact that this
is done in the Mercury runtime rather than by the mdb script, then you
could add an MR_mdb_warning() that uses a "mdb: " prefix rather than a
"Mercury runtime: " prefix, and use that instead of MR_warning().

> +#if defined(HAVE_TCGETATTR) && defined(HAVE_TCSETATTR)
> +	/*
> +	** Turn off echoing before starting the xterm so that
> +	** the user doesn't see the window ID printed by xterm
> +	** on startup (this behaviour is not documented in the
> +	** xterm manual).
> +	*/
> +	tcgetattr(slave_fd, &termio);
> +	termio.c_lflag &= ~ECHO;
> +	tcsetattr(slave_fd, TCSADRAIN, &termio);
...

I think the use of here ECHO and TCSADRAIN may need to be protected by an
appropriate #ifdef.

> +		MR_Code *old_alarm_handler;
...
> +		old_alarm_handler = (MR_Code *) signal(SIGALRM, NULL);
> +		MR_setup_signal_no_restart(SIGALRM,
> +			(MR_Code *) MR_trace_internal_alarm_handler, FALSE,
> +			"error setting up alarm handler");
> +		alarm(10);

A comment here, e.g. "/* 10 second time-out */", might be helpful.

...
> +		/* Reset the alarm handler. */
> +		alarm(0);
> +		MR_setup_signal(SIGALRM, (MR_Code *) old_alarm_handler,
> +			FALSE, "error resetting alarm handler");

Hmm, this is a bit worrying.  Is it safe to call MR_setup_signal()
with the return value from signal()?

Also, the cast to `MR_Code *' here is unnecessary.

> +#if defined(HAVE_TCGETATTR) && defined(HAVE_TCSETATTR)
> +		/* Restore echoing. */
> +		termio.c_lflag |= ECHO;
> +		tcsetattr(slave_fd, TCSADRAIN, &termio);
> +#endif

I think the use of here ECHO and TCSADRAIN may need to be protected by an
appropriate #ifdef.

> +#else 	/* HAVE_OPEN, etc. */

The comment here should say `!HAVE_OPEN, etc.' or `!(HAVE_OPEN && etc.)'.
                             ^
> +#endif /* HAVE_OPEN, etc. */

Likewise.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------