[libvirt] [PATCH] Address missed feedback from review of virt-login-shell
From: "Daniel P. Berrange" <berrange@redhat.com> Address a number of code, style and docs issues identified in review of virt-login-shell after it was merged. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/Makefile.am | 1 - tools/virt-login-shell.c | 58 ++++++++++++++++++++++++++++++---------------- tools/virt-login-shell.pod | 30 ++++++++++++++++++------ 3 files changed, 61 insertions(+), 28 deletions(-) diff --git a/tools/Makefile.am b/tools/Makefile.am index 00c582a..d48883c 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -134,7 +134,6 @@ virt_host_validate_CFLAGS = \ $(NULL) virt_login_shell_SOURCES = \ - virt-login-shell.conf \ virt-login-shell.c virt_login_shell_LDFLAGS = $(COVERAGE_LDFLAGS) diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index b27e44f..1157cd0 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -41,11 +41,11 @@ #include "vircommand.h" #define VIR_FROM_THIS VIR_FROM_NONE -static ssize_t nfdlist = 0; -static int *fdlist = NULL; +static ssize_t nfdlist; +static int *fdlist; static const char *conf_file = SYSCONFDIR "/libvirt/virt-login-shell.conf"; -static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom) +static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom) { size_t i; @@ -105,7 +105,7 @@ static int virLoginShellAllowedUser(virConfPtr conf, } } } - virReportSystemError(EPERM, _("%s not listed as an allowed_users in %s"), name, conf_file); + virReportSystemError(EPERM, _("%s not matched against 'allowed_users' in %s"), name, conf_file); cleanup: VIR_FREE(gname); return ret; @@ -121,7 +121,7 @@ static char **virLoginShellGetShellArgv(virConfPtr conf) if (!p) return virStringSplit("/bin/sh -l", " ", 3); - if (p && p->type == VIR_CONF_LIST) { + if (p->type == VIR_CONF_LIST) { size_t len; virConfValuePtr pp; @@ -139,7 +139,6 @@ static char **virLoginShellGetShellArgv(virConfPtr conf) if (VIR_STRDUP(shargv[i], pp->str) < 0) goto error; } - shargv[len] = NULL; } return shargv; error: @@ -155,16 +154,27 @@ static char *progname; static void usage(void) { - fprintf(stdout, _("\n" - "%s is a privileged program that allows non root users \n" - "specified in %s to join a Linux container \n" - "with a matching user name and launch a shell. \n" - "\n%s [options]\n\n" - " options:\n" - " -h | --help this help:\n\n"), progname, conf_file, progname); + fprintf(stdout, + _("\n" + "Usage:\n" + " %s [options]\n\n" + "Options:\n" + " -h | --help Display program help:\n" + " -V | --version Display program version:\n" + "\n" + "libvirt login shell\n"), + progname); return; } +/* Display version information. */ +static void +show_version(void) +{ + printf("%s (%s) %s\n", progname, PACKAGE_NAME, PACKAGE_VERSION); +} + + int main(int argc, char **argv) { @@ -190,6 +200,7 @@ main(int argc, char **argv) struct option opt[] = { {"help", no_argument, NULL, 'h'}, + {"version", optional_argument, NULL, 'V'}, {NULL, 0, NULL, 0} }; if (virInitialize() < 0) { @@ -214,20 +225,25 @@ main(int argc, char **argv) return ret; } - /* The only option we support is help - */ - while ((arg = getopt_long(argc, argv, "h", opt, &longindex)) != -1) { + while ((arg = getopt_long(argc, argv, "hV", opt, &longindex)) != -1) { switch (arg) { case 'h': usage(); exit(EXIT_SUCCESS); - break; + + case 'V': + show_version(); + exit(EXIT_SUCCESS); + + case '?': + default: + usage(); + exit(EXIT_FAILURE); } } if (argc > optind) { virReportSystemError(EINVAL, _("%s takes no options"), progname); - errno = EINVAL; goto cleanup; } @@ -268,7 +284,9 @@ main(int argc, char **argv) virErrorPtr last_error; last_error = virGetLastError(); if (last_error->code != VIR_ERR_OPERATION_INVALID) { - virReportSystemError(last_error->code,_("Can't create %s container: %s"), name, virGetLastErrorMessage()); + virReportSystemError(last_error->code, + _("Can't create %s container: %s"), + name, last_error->message); goto cleanup; } } @@ -327,7 +345,7 @@ main(int argc, char **argv) } if (execv(shargv[0], (char *const*) shargv) < 0) { virReportSystemError(errno, _("Unable exec shell %s"), shargv[0]); - return -errno; + return EXIT_FAILURE; } } return virProcessWait(ccpid, &status2); diff --git a/tools/virt-login-shell.pod b/tools/virt-login-shell.pod index 0cd35cf..e27d500 100644 --- a/tools/virt-login-shell.pod +++ b/tools/virt-login-shell.pod @@ -8,26 +8,42 @@ B<virt-login-shell> =head1 DESCRIPTION -The B<virt-login-shell> program is setuid shell that is used to join -an LXC container that matches the users name. If the container is not -running virt-login-shell will attempt to start the container. +The B<virt-login-shell> program is a setuid shell that is used to join +an LXC container that matches the user's name. If the container is not +running, virt-login-shell will attempt to start the container. virt-sandbox-shell is not allowed to be run by root. Normal users will get -added to a container that matches their username, if it exists. And they are +added to a container that matches their username, if it exists, and they are configured in /etc/libvirt/virt-login-shell.conf. The basic structure of most virt-login-shell usage is: virt-login-shell +=head1 OPTIONS + +=over + +=item B<-h, --help> + +Display command line help usage then exit. + +=item B<-V, --version> + +Display version information then exit. + +=back + =head1 CONFIG By default, virt-login-shell will execute the /bin/sh program for the user. -You can modify this behaviour by defining the shell variable in /etc/libvirt/virt-login-shell.conf. +You can modify this behaviour by defining the shell variable in +/etc/libvirt/virt-login-shell.conf. eg. shell = [ "/bin/ksh", "--login"] -By default no users are allowed to user virt-login-shell, if you want to allow -certain users to use virt-login-shell, you need to modify the allowed_users variable in /etc/libvirt/virt-login-shell.conf. +By default no users are allowed to use virt-login-shell, if you want to allow +certain users to use virt-login-shell, you need to modify the allowed_users +variable in /etc/libvirt/virt-login-shell.conf. eg. allowed_users = [ "tom", "dick", "harry" ] -- 1.8.3.1
On 08/13/2013 05:16 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Address a number of code, style and docs issues identified in review of virt-login-shell after it was merged.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/Makefile.am | 1 - tools/virt-login-shell.c | 58 ++++++++++++++++++++++++++++++---------------- tools/virt-login-shell.pod | 30 ++++++++++++++++++------ 3 files changed, 61 insertions(+), 28 deletions(-)
ACK.
@@ -327,7 +345,7 @@ main(int argc, char **argv) } if (execv(shargv[0], (char *const*) shargv) < 0) { virReportSystemError(errno, _("Unable exec shell %s"), shargv[0]); - return -errno; + return EXIT_FAILURE;
Setting $? to 1 works, although it's more typical to set to 126 or 127 on execv failure. But I'm fine with it as-is. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 08/13/2013 12:09 PM, Ruben Kerkhof wrote:
On Tue, Aug 13, 2013 at 1:16 PM, Daniel P. Berrange <berrange@redhat.com>wrote:
virReportSystemError(errno, _("Unable exec shell %s"), shargv[0]);
s/Unable/Unable to/
Pushed the fix in your name, along with another line with the same problem ("unable chdir(%s)", and wrapping some long lines: From 11cdc424d30b15c6780d546a2f0d8ff93ce291b6 Mon Sep 17 00:00:00 2001 From: Ruben Kerkhof <ruben@rubenkerkhof.com> Date: Tue, 13 Aug 2013 17:28:06 -0600 Subject: [PATCH] virt-login-shell: improve error message grammar and wrap some long lines Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virt-login-shell.c | 18 ++++++++++++------ 1 files changed, 12 insertions(+), 6 deletions(-) diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index 1157cd0..c754ae4 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -76,7 +76,8 @@ static int virLoginShellAllowedUser(virConfPtr conf, /* Calc length and check items */ for (pp = p->list; pp; pp = pp->next) { if (pp->type != VIR_CONF_STRING) { - virReportSystemError(EINVAL, "%s", _("shell must be a list of strings")); + virReportSystemError(EINVAL, "%s", + _("shell must be a list of strings")); goto cleanup; } else { /* @@ -105,7 +106,9 @@ static int virLoginShellAllowedUser(virConfPtr conf, } } } - virReportSystemError(EPERM, _("%s not matched against 'allowed_users' in %s"), name, conf_file); + virReportSystemError(EPERM, + _("%s not matched against 'allowed_users' in %s"), + name, conf_file); cleanup: VIR_FREE(gname); return ret; @@ -128,7 +131,8 @@ static char **virLoginShellGetShellArgv(virConfPtr conf) /* Calc length and check items */ for (len = 0, pp = p->list; pp; len++, pp = pp->next) { if (pp->type != VIR_CONF_STRING) { - virReportSystemError(EINVAL, "%s", _("shell must be a list of strings")); + virReportSystemError(EINVAL, "%s", + _("shell must be a list of strings")); goto error; } } @@ -248,7 +252,8 @@ main(int argc, char **argv) } if (uid == 0) { - virReportSystemError(EPERM, _("%s must be run by non root users"), progname); + virReportSystemError(EPERM, _("%s must be run by non root users"), + progname); goto cleanup; } @@ -340,11 +345,12 @@ main(int argc, char **argv) if (ccpid == 0) { if (chdir(homedir) < 0) { - virReportSystemError(errno, _("Unable chdir(%s)"), homedir); + virReportSystemError(errno, _("Unable to chdir(%s)"), homedir); return EXIT_FAILURE; } if (execv(shargv[0], (char *const*) shargv) < 0) { - virReportSystemError(errno, _("Unable exec shell %s"), shargv[0]); + virReportSystemError(errno, _("Unable to exec shell %s"), + shargv[0]); return EXIT_FAILURE; } } -- 1.7.1 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Aug 14, 2013 at 1:35 AM, Eric Blake <eblake@redhat.com> wrote:
Pushed the fix in your name, along with another line with the same problem ("unable chdir(%s)", and wrapping some long lines:
From 11cdc424d30b15c6780d546a2f0d8ff93ce291b6 Mon Sep 17 00:00:00 2001 From: Ruben Kerkhof <ruben@rubenkerkhof.com> Date: Tue, 13 Aug 2013 17:28:06 -0600 Subject: [PATCH] virt-login-shell: improve error message grammar
and wrap some long lines
Signed-off-by: Eric Blake <eblake@redhat.com>
Great, thanks! Ruben
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Ruben Kerkhof