On Thu, Oct 13, 2022 at 11:48:18AM +0200, Stefano Brivio wrote:On Thu, 13 Oct 2022 19:22:27 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Ah, right, that makes sense. Adjusted.On Thu, Oct 13, 2022 at 04:16:59AM +0200, Stefano Brivio wrote:const struct pasta_setup_ns_arg *a; a = (const struct pasta_setup_ns_arg *)arg;Just nits here: On Tue, 11 Oct 2022 16:40:10 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Uh.. I'm not sure what you mean by that.When invoked so as to spawn a shell, pasta checks explicitly for the shell being bash and if so, adds a "-l" option to make it a login shell. This is not ideal, since this is a bash specific option and requires pasta to know about specific shell variants. There's a general convention for starting a login shell, which is to prepend a "-" to argv[0]. Use this approach instead, so we don't need bash specific logic.Hah, I didn't know that was the meaning.Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- pasta.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/pasta.c b/pasta.c index 1dd8267..7c3acef 100644 --- a/pasta.c +++ b/pasta.c @@ -148,10 +148,12 @@ void pasta_open_ns(struct ctx *c, const char *netns) /** * struct pasta_setup_ns_arg - Argument for pasta_setup_ns() + * @exe: Executable to run * @argv: Command and arguments to run */ struct pasta_setup_ns_arg { - char **argv; + const char *exe; + char *const *argv; }; /** @@ -162,12 +164,13 @@ struct pasta_setup_ns_arg { */ static int pasta_setup_ns(void *arg) { - struct pasta_setup_ns_arg *a = (struct pasta_setup_ns_arg *)arg; + const struct pasta_setup_ns_arg *a + = (const struct pasta_setup_ns_arg *)arg;At this point the assignment could be split onto another line.I see what you mean about the crampedness. But the proposed alternative seems more complicated in other ways to me. I think I'll leave it as is for now. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibsonAh, sorry. if ((len = strlen(arg.exe)) + 1 /* - */ + 1 /* \0 */ > sizeof(sh_arg0)) err("$SHELL is too long (%u bytes)", strlen(arg.exe)); (void)snprintf(sh_arg0, sizeof(sh_arg0), "-%s", arg.exe); I don't know, it doesn't look so... cramped. Maybe it's just me. If it doesn't make sense just disregard ;)Done.FWRITE("/proc/sys/net/ipv4/ping_group_range", "0 0", "Cannot set ping_group_range, ICMP requests might fail"); - execvp(a->argv[0], a->argv); + execvp(a->exe, a->argv); perror("execvp"); exit(EXIT_FAILURE); @@ -182,26 +185,31 @@ static int pasta_setup_ns(void *arg) void pasta_start_ns(struct ctx *c, int argc, char *argv[]) { struct pasta_setup_ns_arg arg = { + .exe = argv[0], .argv = argv, }; - char *shell = getenv("SHELL"); - char *sh_argv[] = { shell, NULL }; - char *bash_argv[] = { shell, "-l", NULL }; + char *sh_argv[] = { NULL, NULL }; char ns_fn_stack[NS_FN_STACK_SIZE];If you respin, it would be nice to have the usual ordering here (sh_argv[] after ns_fn_stack).Uh.. not sure what you mean by that either.+ char sh_arg0[PATH_MAX + 1]; c->foreground = 1; if (!c->debug) c->quiet = 1; - if (!shell) - shell = "/bin/sh"; if (argc == 0) { - if (strstr(shell, "/bash")) { - arg.argv = bash_argv; - } else { - arg.argv = sh_argv; + arg.exe = getenv("SHELL"); + if (!arg.exe) + arg.exe = "/bin/sh"; + + if ((size_t)snprintf(sh_arg0, sizeof(sh_arg0), + "-%s", arg.exe) >= sizeof(sh_arg0)) {This is completely specified and looks safe, but it also looks more complicated than it actually is, at a glance. Maybe a separate length check before snprintf() would make it look more natural. Not a strong preference though.