[libvirt] [PATCH] Address missed feedback from review of virt-login-shell

Daniel P. Berrange berrange at redhat.com
Tue Aug 13 11:16:52 UTC 2013


From: "Daniel P. Berrange" <berrange at 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 at 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




More information about the libvir-list mailing list