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

[Daniel P. Berrange]

On Wed, Apr 27, 2016 at 02:28:33PM -0400, John Ferlan wrote:
> 
> 
> 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 at redhat.com>
> > ---
> >  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.

Yep.

> >      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?

The struct option entry is only required if we need to support a
--long argument alternative. These two only need short args so
are omitted here.

> ACK with the changes -

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|