[m-rev.] for review: improve `mdb --window'
Simon Taylor
stayl at cs.mu.OZ.AU
Wed Jan 30 03:21:59 AEDT 2002
On 29-Jan-2002, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> > 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 */ {
> ...
> }
That won't work because the `if (MR_mdb_in_window)' part sets
MR_mdb_in_window to FALSE if opening the window failed. I've
added a comment.
>
> 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().
Done.
> 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().
Done.
> ...
> > + /* 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()?
Probably not. I've added functions to mercury_signal.h to
get and set signal actions.
diff -u runtime/mercury_signal.c runtime/mercury_signal.c
--- runtime/mercury_signal.c
+++ runtime/mercury_signal.c
@@ -98,7 +98,7 @@
}
void
-MR_setup_signal_no_restart(int sig, MR_Code *handler, bool need_info,
+MR_setup_signal_no_restart(int sig, MR_Code *handler, bool need_info,
const char *error_message)
{
MR_do_setup_signal(sig, handler, need_info, FALSE, error_message);
@@ -108,9 +108,9 @@
MR_do_setup_signal(int sig, MR_Code *handler, bool need_info, bool restart,
const char *error_message)
{
-#if defined(HAVE_SIGACTION)
+ MR_signal_action act;
- struct sigaction act;
+#if defined(HAVE_SIGACTION)
act.sa_flags = (restart ? SA_RESTART : 0);
@@ -126,23 +126,53 @@
#endif
}
if (sigemptyset(&act.sa_mask) != 0) {
- perror("Mercury runtime: cannot set clear signal mask");
+ MR_perror("Mercury runtime: cannot set clear signal mask");
exit(1);
}
errno = 0;
act.SIGACTION_FIELD = handler;
- if (sigaction(sig, &act, NULL) != 0) {
- perror(error_message);
+#else /* not HAVE_SIGACTION */
+
+ act = handler;
+
+#endif /* not HAVE_SIGACTION */
+
+ MR_set_signal_action(sig, &act, error_message);
+}
+
+void
+MR_get_signal_action(int sig, MR_signal_action *act,
+ const char *error_message)
+{
+#ifdef HAVE_SIGACTION
+ if (sigaction(sig, NULL, act) != 0) {
+ MR_perror(error_message);
exit(1);
}
#else /* not HAVE_SIGACTION */
-
- if (signal(sig, handler) == SIG_ERR) {
- perror(error_message);
+ if ((*act = signal(sig, NULL)) == SIG_ERR) {
+ MR_perror(error_message);
exit(1);
}
#endif /* not HAVE_SIGACTION */
}
+void
+MR_set_signal_action(int sig, MR_signal_action *act,
+ const char *error_message)
+{
+#ifdef HAVE_SIGACTION
+ if (sigaction(sig, act, NULL) != 0) {
+ MR_perror(error_message);
+ exit(1);
+ }
+
+#else /* not HAVE_SIGACTION */
+ if (signal(sig, *act) == SIG_ERR) {
+ MR_perror(error_message);
+ exit(1);
+ }
+#endif /* not HAVE_SIGACTION */
+}
diff -u runtime/mercury_signal.h runtime/mercury_signal.h
--- runtime/mercury_signal.h
+++ runtime/mercury_signal.h
@@ -15,6 +15,13 @@
#include "mercury_types.h"
#include "mercury_std.h"
+#include "mercury_conf.h"
+
+#ifdef HAVE_SIGACTION
+typedef struct sigaction MR_signal_action;
+#else
+typedef MR_Code * MR_signal_action;
+#endif
/*
** MR_setup_signal sets a signal handler (handler) to handle
@@ -36,5 +43,22 @@
*/
extern void MR_setup_signal_no_restart(int sig, MR_Code *handler,
bool need_info, const char * error_message);
+
+ /*
+ ** Get the current action for the given signal.
+ ** If the action cannot be retrieved, it aborts with the given
+ ** error message.
+ */
+extern void MR_get_signal_action(int sig, MR_signal_action *old_action,
+ const char *error_message);
+
+ /*
+ ** Restore the action for the given signal to the result
+ ** of a previous call to MR_get_signal_action().
+ ** If the action cannot be set, it aborts with the given
+ ** error message.
+ */
+extern void MR_set_signal_action(int sig, MR_signal_action *action,
+ const char *error_message);
#endif /* not MERCURY_SIGNAL_H */
diff -u runtime/mercury_trace_base.c runtime/mercury_trace_base.c
--- runtime/mercury_trace_base.c
+++ runtime/mercury_trace_base.c
@@ -222,6 +222,9 @@
#endif
#if defined(HAVE_KILL) && defined(HAVE_WAIT) && defined(SIGTERM)
+ /*
+ ** If mdb started a window, make sure it dies now.
+ */
if (MR_have_mdb_window) {
int status;
status = kill(MR_mdb_window_pid, SIGTERM);
diff -u trace/mercury_trace_internal.c trace/mercury_trace_internal.c
--- trace/mercury_trace_internal.c
+++ trace/mercury_trace_internal.c
@@ -394,10 +394,15 @@
int n;
if (MR_mdb_in_window) {
+ /*
+ ** If opening the window fails, fall back on
+ ** using MR_mdb_*_filename, or stdin, stdout
+ ** and stderr.
+ */
MR_mdb_in_window =
MR_trace_internal_create_mdb_window();
if (! MR_mdb_in_window) {
- fprintf(stderr,
+ MR_mdb_warning(
"Try `mdb --program-in-window' instead.\n");
}
}
@@ -436,7 +441,7 @@
}
}
-static bool MR_got_alarm = FALSE;
+static volatile sig_atomic_t MR_got_alarm = FALSE;
static void
MR_trace_internal_alarm_handler(void)
@@ -448,48 +453,50 @@
MR_trace_internal_create_mdb_window(void)
{
/*
+ ** XXX The 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).
+ **
** 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)
+#if defined(HAVE_OPEN) && defined(O_RDWR) && defined(HAVE_FDOPEN) && \
+ defined(HAVE_CLOSE) && defined(HAVE_DUP) && defined(HAVE_DUP2) && \
+ defined(HAVE_FORK) && defined(HAVE_EXECLP) && \
+ defined(HAVE_DEV_PTMX) && defined(HAVE_GRANTPT) && \
+ defined(HAVE_UNLOCKPT) && defined(HAVE_PTSNAME)
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)
+ defined(HAVE_TCSETATTR) && defined(ECHO) && defined(TCSADRAIN)
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).
- */
-#if defined(HAVE_DEV_PTMX) && defined(HAVE_GRANTPT) && \
- defined(HAVE_UNLOCKPT) && defined(HAVE_PTSNAME)
master_fd = open("/dev/ptmx", O_RDWR);
if (master_fd == -1 || grantpt(master_fd) == -1
- || unlockpt(master_fd) == -1) {
+ || unlockpt(master_fd) == -1)
+ {
close(master_fd);
- perror("error opening master pseudo-terminal for mdb window");
+ MR_mdb_perror(
+ "error opening master pseudo-terminal for mdb window");
return FALSE;
}
if ((slave_name = ptsname(master_fd)) == NULL) {
- perror("error getting name of pseudo-terminal for mdb window");
+ MR_mdb_perror(
+ "error getting name of pseudo-terminal for mdb window");
close(master_fd);
return FALSE;
}
slave_fd = open(slave_name, O_RDWR);
if (slave_fd == -1) {
close(master_fd);
- perror("opening slave pseudo-terminal for mdb window failed");
+ MR_mdb_perror(
+ "opening slave pseudo-terminal for mdb window failed");
return FALSE;
}
@@ -500,13 +507,8 @@
ioctl(slave_fd, I_PUSH, "ttcompat");
#endif
-#else
- fprintf(stderr,
- "Sorry, `mdb --window' not supported on this platform.\n");
- return FALSE;
-#endif
-
-#if defined(HAVE_TCGETATTR) && defined(HAVE_TCSETATTR)
+#if defined(HAVE_TCGETATTR) && defined(HAVE_TCSETATTR) && \
+ defined(ECHO) && defined(TCSADRAIN)
/*
** Turn off echoing before starting the xterm so that
** the user doesn't see the window ID printed by xterm
@@ -520,7 +522,7 @@
child_pid = fork();
if (child_pid == -1) {
- perror("fork() for mdb window failed");
+ MR_mdb_perror("fork() for mdb window failed");
close(master_fd);
close(slave_fd);
return FALSE;
@@ -538,7 +540,7 @@
** killed by SIGINT signals sent to the program.
*/
if (setpgid(0, 0) < 0) {
- perror("setpgid() failed");
+ MR_mdb_perror("setpgid() failed");
close(master_fd);
exit(EXIT_FAILURE);
}
@@ -559,14 +561,14 @@
sprintf(xterm_arg, "-SXX%d", master_fd);
execlp("xterm", "xterm", "-T", "mdb", xterm_arg, NULL);
- perror("execlp() of xterm failed");
+ MR_mdb_perror("execution of xterm failed");
exit(EXIT_FAILURE);
} else {
/*
** Parent - set up the mdb I/O streams to point
** to the pseudo-terminal.
*/
- MR_Code *old_alarm_handler;
+ MR_signal_action old_alarm_action;
int wait_status;
int err_fd = -1;
int out_fd = -1;
@@ -582,19 +584,20 @@
** start, for example because the DISPLAY variable was invalid.
** We don't want to restart the read() below if it times out.
*/
- old_alarm_handler = (MR_Code *) signal(SIGALRM, NULL);
+ MR_get_signal_action(SIGALRM, &old_alarm_action,
+ "error retrieving alarm handler");
MR_setup_signal_no_restart(SIGALRM,
- (MR_Code *) MR_trace_internal_alarm_handler, FALSE,
+ MR_trace_internal_alarm_handler, FALSE,
"error setting up alarm handler");
- alarm(10);
+ alarm(10); /* 10 second timeout */
while (1) {
char c;
int status;
status = read(slave_fd, &c, 1);
if (status == -1) {
if (errno != EINTR || MR_got_alarm) {
- perror(
- "error reading from pseudo-terminal");
+ MR_mdb_perror(
+ "error reading from mdb window");
goto parent_error;
}
} else if (status == 0 || c == '\n') {
@@ -604,39 +607,43 @@
/* Reset the alarm handler. */
alarm(0);
- MR_setup_signal(SIGALRM, (MR_Code *) old_alarm_handler,
- FALSE, "error resetting alarm handler");
+ MR_set_signal_action(SIGALRM, &old_alarm_action,
+ "error resetting alarm handler");
-#if defined(HAVE_TCGETATTR) && defined(HAVE_TCSETATTR)
+#if defined(HAVE_TCGETATTR) && defined(HAVE_TCSETATTR) && \
+ defined(ECHO) && defined(TCSADRAIN)
/* Restore echoing. */
termio.c_lflag |= ECHO;
tcsetattr(slave_fd, TCSADRAIN, &termio);
#endif
if ((out_fd = dup(slave_fd)) == -1) {
- perror(
+ MR_mdb_perror(
"opening slave pseudo-terminal for xterm failed");
goto parent_error;
}
if ((err_fd = dup(slave_fd)) == -1) {
- perror(
+ MR_mdb_perror(
"opening slave pseudo-terminal for xterm failed");
goto parent_error;
}
MR_mdb_in = fdopen(slave_fd, "r");
if (MR_mdb_in == NULL) {
- perror("opening slave pseudo-terminal for xterm failed");
+ MR_mdb_perror(
+ "opening slave pseudo-terminal for xterm failed");
goto parent_error;
}
MR_mdb_out = fdopen(out_fd, "w");
if (MR_mdb_out == NULL) {
- perror("opening slave pseudo-terminal for xterm failed");
+ MR_mdb_perror(
+ "opening slave pseudo-terminal for xterm failed");
goto parent_error;
}
MR_mdb_err = fdopen(err_fd, "w");
if (MR_mdb_err == NULL) {
- perror("opening slave pseudo-terminal for xterm failed");
+ MR_mdb_perror(
+ "opening slave pseudo-terminal for xterm failed");
goto parent_error;
}
@@ -665,11 +672,11 @@
}
-#else /* HAVE_OPEN, etc. */
- fprintf(stderr,
+#else /* !HAVE_OPEN, etc. */
+ MR_mdb_warning(
"Sorry, `mdb --window' not supported on this platform.\n");
return FALSE;
-#endif /* HAVE_OPEN, etc. */
+#endif /* !HAVE_OPEN, etc. */
}
static void
only in patch2:
--- runtime/mercury_misc.h 8 May 2000 14:00:59 -0000 1.20
+++ runtime/mercury_misc.h 29 Jan 2002 13:17:42 -0000
@@ -16,6 +16,15 @@
#include <stdlib.h> /* for `size_t' */
extern void MR_warning(const char *msg, ...);
+
+ /* For warnings from the debugger */
+extern void MR_mdb_warning(const char *msg, ...);
+
+extern void MR_perror(const char *msg);
+
+ /* For errors from the debugger */
+extern void MR_mdb_perror(const char *msg);
+
extern void MR_fatal_error(const char *msg, ...) NO_RETURN;
#endif /* not MERCURY_MISC_H */
only in patch2:
--- runtime/mercury_misc.c 3 Aug 2000 06:18:50 -0000 1.23
+++ runtime/mercury_misc.c 29 Jan 2002 14:13:30 -0000
@@ -13,21 +13,68 @@
#include <stdio.h>
#include <stdarg.h>
+#include <errno.h>
+
+static void MR_print_warning(const char *prog, const char *fmt, va_list args);
+static void MR_do_perror(const char *prog, const char *message);
void
MR_warning(const char *fmt, ...)
{
va_list args;
+ va_start(args, fmt);
+ MR_print_warning("Mercury runtime", fmt, args);
+ va_end(args);
+
+}
+
+void
+MR_mdb_warning(const char *fmt, ...)
+{
+ va_list args;
+ va_start(args, fmt);
+ MR_print_warning("mdb", fmt, args);
+ va_end(args);
+}
+
+static void
+MR_print_warning(const char *prog, const char *fmt, va_list args)
+{
fflush(stdout); /* in case stdout and stderr are the same */
- fprintf(stderr, "Mercury runtime: ");
- va_start(args, fmt);
+ fprintf(stderr, prog);
+ fprintf(stderr, ": ");
vfprintf(stderr, fmt, args);
- va_end(args);
fprintf(stderr, "\n");
fflush(stderr);
+}
+
+void
+MR_perror(const char *message)
+{
+ MR_do_perror("Mercury runtime", message);
+}
+
+void
+MR_mdb_perror(const char *message)
+{
+ MR_do_perror("mdb", message);
+}
+
+static void
+MR_do_perror(const char *prog, const char *message)
+{
+ int saved_errno;
+
+ saved_errno = errno;
+ fflush(stdout); /* in case stdout and stderr are the same */
+
+ fprintf(stderr, prog);
+ fprintf(stderr, ": ");
+ errno = saved_errno;
+ perror(message);
}
/*
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------
More information about the reviews
mailing list