Re: [libvirt] [PATCH 11/17] virt-login-shell: honour the -c option to launch commands

On 04/14/2016 11:22 AM, Daniel P. Berrange wrote: > The virt-login-shell program is supposed to look like a > regular shell to clients. Login services like sshd > expect the shell to accept a '-c cmdstring' argument to > specify a command to launch instead of presenting an > interactive prompt. > > We can implement this by simply passing the '-c cmdstring' > data straight through to the real shell we use. This does > not open any security holes, since the command is not run > until we're inside the container namespaces. This allows > scp to work for users with virt-login-shell. > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > --- > tools/virt-login-shell.c | 70 +++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 54 insertions(+), 16 deletions(-) > > diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c > index ec759dc..34a8836 100644 > --- a/tools/virt-login-shell.c > +++ b/tools/virt-login-shell.c > @@ -100,20 +100,19 @@ static int virLoginShellAllowedUser(virConfPtr conf, > return ret; > } > > -static char **virLoginShellGetShellArgv(virConfPtr conf) > +static int virLoginShellGetShellArgv(virConfPtr conf, > + char ***retshargv, > + size_t *retshargvlen) > { > size_t i; > + size_t len; > char **shargv = NULL; > - virConfValuePtr p; > + virConfValuePtr p, pp; > > p = virConfGetValue(conf, "shell"); > - if (!p) > - return virStringSplit("/bin/sh -l", " ", 3); > - > - if (p->type == VIR_CONF_LIST) { > - size_t len; > - virConfValuePtr pp; > - > + if (!p) { > + len = 2; /* /bin/sh -l */ > + } else if (p->type == VIR_CONF_LIST) { > /* Calc length and check items */ > for (len = 0, pp = p->list; pp; len++, pp = pp->next) { > if (pp->type != VIR_CONF_STRING) { > @@ -122,18 +121,41 @@ static char **virLoginShellGetShellArgv(virConfPtr conf) > goto error; > } > } > + } else { > + virReportSystemError(EINVAL, "%s", > + _("shell must be a list of strings")); > + goto error; > + } > > - if (VIR_ALLOC_N(shargv, len + 1) < 0) > + len++; /* NULL terminator */ > + > + if (VIR_ALLOC_N(shargv, len) < 0) > + goto error; > + > + i = 0; > + if (!p) { > + if (VIR_STRDUP(shargv[i++], "/bin/sh") < 0) > goto error; > - for (i = 0, pp = p->list; pp; i++, pp = pp->next) { > - if (VIR_STRDUP(shargv[i], pp->str) < 0) > + if (VIR_STRDUP(shargv[i++], "-l") < 0) > + goto error; > + } else if (p->type == VIR_CONF_LIST) { > + for (pp = p->list; pp; pp = pp->next) { > + if (VIR_STRDUP(shargv[i++], pp->str) < 0) > goto error; > } > } > - return shargv; > + > + shargv[i] = NULL; > + > + *retshargvlen = i; > + *retshargv = shargv; > + > + return 0; > error: > + *retshargv = NULL; > + *retshargvlen = 0; > virStringFreeList(shargv); > - return NULL; > + return -1; > } > > static char *progname; Right in here somewhere there's some 'usage()' that should probably be updated.

> struct option opt[] = { > {"help", no_argument, NULL, 'h'}, Should this add a "-c" w/ required_argument? I also see the "V" lists "optional_argument", but that's not part of the "hV" below, so shouldn't it be no_argument?

ACK with the changes -