~emersion/mrsh-dev

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
1

[PATCH] Implement 'wait' builtin

Details
Message ID
<20190604234354.18489-1-sir@cmpwn.com>
Sender timestamp
1559691834
DKIM signature
pass
Download raw message
Patch: +112 -0
---
 builtin/builtin.c |   1 +
 builtin/wait.c    | 109 ++++++++++++++++++++++++++++++++++++++++++++++
 include/builtin.h |   1 +
 meson.build       |   1 +
 4 files changed, 112 insertions(+)
 create mode 100644 builtin/wait.c

diff --git a/builtin/builtin.c b/builtin/builtin.c
index aaeac2b..ce77af3 100644
--- a/builtin/builtin.c
@@ -38,6 +38,7 @@ static const struct builtin builtins[] = {
 	{ "umask", builtin_umask, false },
 	{ "unalias", builtin_unalias, false },
 	{ "unset", builtin_unset, true },
+	{ "wait", builtin_wait, false },
 };
 
 // The following commands are explicitly unspecified by POSIX
diff --git a/builtin/wait.c b/builtin/wait.c
new file mode 100644
index 0000000..1c610df
--- /dev/null
@@ -0,0 +1,109 @@
+#define _POSIX_C_SOURCE 200112L
+#include <errno.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/wait.h>
+#include "builtin.h"
+#include "shell/process.h"
+#include "shell/shell.h"
+
+struct wait_handle {
+	pid_t pid;
+	int status;
+};
+
+int builtin_wait(struct mrsh_state *state, int argc, char *argv[]) {
+	int npids = argc - 1;
+	if (npids == 0) {
+		npids = state->processes.len;
+	}
+	struct wait_handle *pids = malloc(npids * sizeof(struct wait_handle));
+	if (pids == NULL) {
+		fprintf(stderr, "wait: unable to allocate pid list");
+		return EXIT_FAILURE;
+	}
+
+	if (argc == 1) {
+		/* All known processes */
+		int _npids = 0;
+		for (size_t j = 0; j < state->processes.len; ++j) {
+			struct process *process = state->processes.data[j];
+			if (process->terminated) {
+				continue;
+			}
+			pids[_npids].pid = process->pid;
+			pids[_npids].status = -1;
+			++_npids;
+		}
+		npids = _npids;
+	} else {
+		for (int i = 1; i < argc; ++i) {
+			if (argv[i][0] == '%') {
+				// TODO
+				fprintf(stderr, "wait: job control IDs are unimplemented\n");
+				return EXIT_FAILURE;
+			} else {
+				char *endptr;
+				pid_t pid = (pid_t)strtol(argv[i], &endptr, 10);
+				if (*endptr != '\0') {
+					fprintf(stderr, "wait: error parsing pid '%s: %s",
+							argv[i], strerror(errno));
+					return EXIT_FAILURE;
+				}
+				if (pid < 0) {
+					fprintf(stderr, "wait: cannot wait on negative pid\n");
+					return EXIT_FAILURE;
+				}
+				pids[i - 1].pid = pid;
+				pids[i - 1].status = -1;
+				/* Check if this pid is known */
+				bool found = false;
+				for (size_t j = 0; j < state->processes.len; ++j) {
+					struct process *process = state->processes.data[j];
+					if (process->pid == pid) {
+						if (process->terminated) {
+							pids[i].status = process->stat;
+						}
+						found = true;
+						break;
+					}
+				}
+				if (!found) {
+					/* Unknown pids are assumed to have exited 127 */
+					pids[i - 1].status = 127;
+				}
+			}
+		}
+	}
+
+	bool loop = true;
+	while (loop) {
+		int stat;
+		pid_t waited = waitpid((pid_t)-1, &stat, 0);
+		if (waited == -1) {
+			if (errno == ECHILD) {
+				/* No remaining child processes */
+				break;
+			}
+			fprintf(stderr, "wait: %s\n", strerror(errno));
+			return EXIT_FAILURE;
+		}
+		loop = false;
+		for (int i = 0; i < npids; ++i) {
+			if (pids[i].pid == waited) {
+				pids[i].status = WEXITSTATUS(stat);
+			}
+			if (pids[i].status == -1) {
+				loop = true;
+			}
+		}
+	}
+
+	if (argc == 1) {
+		return EXIT_SUCCESS;
+	} else {
+		return pids[npids - 1].status;
+	}
+}
diff --git a/include/builtin.h b/include/builtin.h
index fb1933a..1f60125 100644
--- a/include/builtin.h
+++ b/include/builtin.h
@@ -31,6 +31,7 @@ int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_umask(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unalias(struct mrsh_state *state, int argc, char *argv[]);
 int builtin_unset(struct mrsh_state *state, int argc, char *argv[]);
+int builtin_wait(struct mrsh_state *state, int argc, char *argv[]);
 
 int builtin_unspecified(struct mrsh_state *state, int argc, char *argv[]);
 
diff --git a/meson.build b/meson.build
index e9b43e8..9c1f67a 100644
--- a/meson.build
+++ b/meson.build
@@ -97,6 +97,7 @@ lib_mrsh = library(
 		'builtin/unalias.c',
 		'builtin/unset.c',
 		'builtin/unspecified.c',
+		'builtin/wait.c',
 		'getopt.c',
 		'hashtable.c',
 		'parser/arithm.c',
-- 
2.21.0
Details
Message ID
<lcFnhD5I5gewMt-f__WDFTAkMLgwlhDZJzGiC3BP6i0rkWa_6p2-c3fWGz0N4ipk55pCog1bgDvZSeReunz8ADHh3cq3SCyZEDOg6gwor_s=@emersion.fr>
In-Reply-To
<20190604234354.18489-1-sir@cmpwn.com> (view parent)
Sender timestamp
1559928016
DKIM signature
pass
Download raw message
> ---
>  builtin/builtin.c |   1 +
>  builtin/wait.c    | 109 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/builtin.h |   1 +
>  meson.build       |   1 +
>  4 files changed, 112 insertions(+)
>  create mode 100644 builtin/wait.c
>
> diff --git a/builtin/builtin.c b/builtin/builtin.c
> index aaeac2b..ce77af3 100644
> --- a/builtin/builtin.c
> +++ b/builtin/builtin.c
> @@ -38,6 +38,7 @@ static const struct builtin builtins[] = {
>  	{ "umask", builtin_umask, false },
>  	{ "unalias", builtin_unalias, false },
>  	{ "unset", builtin_unset, true },
> +	{ "wait", builtin_wait, false },
>  };
>
>  // The following commands are explicitly unspecified by POSIX
> diff --git a/builtin/wait.c b/builtin/wait.c
> new file mode 100644
> index 0000000..1c610df
> --- /dev/null
> +++ b/builtin/wait.c
> @@ -0,0 +1,109 @@
> +#define _POSIX_C_SOURCE 200112L
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/wait.h>
> +#include "builtin.h"
> +#include "shell/process.h"
> +#include "shell/shell.h"
> +
> +struct wait_handle {
> +	pid_t pid;
> +	int status;
> +};
> +
> +int builtin_wait(struct mrsh_state *state, int argc, char *argv[]) {
> +	int npids = argc - 1;
> +	if (npids == 0) {
> +		npids = state->processes.len;
> +	}
> +	struct wait_handle *pids = malloc(npids * sizeof(struct wait_handle));

Need to free this at some point.

> +	if (pids == NULL) {
> +		fprintf(stderr, "wait: unable to allocate pid list");
> +		return EXIT_FAILURE;
> +	}
> +
> +	if (argc == 1) {
> +		/* All known processes */

I think we only want to wait processes which are asynchronous lists. We also
monitor other processes, e.g. command substitutions.

Though it's probably fine anyway, since all of these would have finished anyway
by the time the builtin is called.

> +		int _npids = 0;
> +		for (size_t j = 0; j < state->processes.len; ++j) {
> +			struct process *process = state->processes.data[j];
> +			if (process->terminated) {
> +				continue;
> +			}
> +			pids[_npids].pid = process->pid;
> +			pids[_npids].status = -1;
> +			++_npids;
> +		}
> +		npids = _npids;
> +	} else {
> +		for (int i = 1; i < argc; ++i) {
> +			if (argv[i][0] == '%') {
> +				// TODO
> +				fprintf(stderr, "wait: job control IDs are unimplemented\n");
> +				return EXIT_FAILURE;
> +			} else {
> +				char *endptr;
> +				pid_t pid = (pid_t)strtol(argv[i], &endptr, 10);
> +				if (*endptr != '\0') {

Also need to check that argv[i][0] != '\0'.

> +					fprintf(stderr, "wait: error parsing pid '%s: %s",
> +							argv[i], strerror(errno));

errno won't be set if the string begins with a valid number (e.g. "1234abcd").
Probably better to not print it at all.

> +					return EXIT_FAILURE;
> +				}
> +				if (pid < 0) {

pid 0 is invalid too.

> +					fprintf(stderr, "wait: cannot wait on negative pid\n");
> +					return EXIT_FAILURE;
> +				}
> +				pids[i - 1].pid = pid;
> +				pids[i - 1].status = -1;
> +				/* Check if this pid is known */
> +				bool found = false;
> +				for (size_t j = 0; j < state->processes.len; ++j) {
> +					struct process *process = state->processes.data[j];
> +					if (process->pid == pid) {
> +						if (process->terminated) {
> +							pids[i].status = process->stat;

Should be pids[i - 1].

> +						}
> +						found = true;
> +						break;
> +					}
> +				}
> +				if (!found) {
> +					/* Unknown pids are assumed to have exited 127 */
> +					pids[i - 1].status = 127;
> +				}
> +			}
> +		}
> +	}
> +
> +	bool loop = true;
> +	while (loop) {
> +		int stat;
> +		pid_t waited = waitpid((pid_t)-1, &stat, 0);

waitpid can't be used directly here. We need to our update jobs/processes
internal state when retrieving information from a process.

We could add a function similar to job_wait, which works like waitpid but also
updates our internal state.

Additionally, POSIX says:

> However, the wording means that wait is required to wait for an explicit
> process when it is given an argument so that the status information of other
> processes is not consumed.

But I think it's fine to waitpid with (pid_t)-1 as long as we update our
internal state.

> +		if (waited == -1) {
> +			if (errno == ECHILD) {
> +				/* No remaining child processes */
> +				break;
> +			}
> +			fprintf(stderr, "wait: %s\n", strerror(errno));
> +			return EXIT_FAILURE;
> +		}
> +		loop = false;
> +		for (int i = 0; i < npids; ++i) {
> +			if (pids[i].pid == waited) {
> +				pids[i].status = WEXITSTATUS(stat);

We can only call this macro if WIFEXITED(stat). Otherwise, the spec says that:

> If the process terminated abnormally due to the receipt of a signal, the exit
> status shall be greater than 128 and shall be distinct from the exit status
> generated by other signals, but the exact value is unspecified.

> +			}
> +			if (pids[i].status == -1) {
> +				loop = true;
> +			}
> +		}
> +	}
> +
> +	if (argc == 1) {
> +		return EXIT_SUCCESS;
> +	} else {
> +		return pids[npids - 1].status;

npids could be zero.

> +	}
> +}
> diff --git a/include/builtin.h b/include/builtin.h
> index fb1933a..1f60125 100644
> --- a/include/builtin.h
> +++ b/include/builtin.h
> @@ -31,6 +31,7 @@ int builtin_ulimit(struct mrsh_state *state, int argc, char *argv[]);
>  int builtin_umask(struct mrsh_state *state, int argc, char *argv[]);
>  int builtin_unalias(struct mrsh_state *state, int argc, char *argv[]);
>  int builtin_unset(struct mrsh_state *state, int argc, char *argv[]);
> +int builtin_wait(struct mrsh_state *state, int argc, char *argv[]);
>
>  int builtin_unspecified(struct mrsh_state *state, int argc, char *argv[]);
>
> diff --git a/meson.build b/meson.build
> index e9b43e8..9c1f67a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -97,6 +97,7 @@ lib_mrsh = library(
>  		'builtin/unalias.c',
>  		'builtin/unset.c',
>  		'builtin/unspecified.c',
> +		'builtin/wait.c',
>  		'getopt.c',
>  		'hashtable.c',
>  		'parser/arithm.c',
> --
> 2.21.0