From b76dda6c153e1251de6aa935b85942bb53213470 Mon Sep 17 00:00:00 2001

Message-Id: <b76dda6c153e1251de6aa935b85942bb53213470.1377873639.git.jdenemar@redhat.com>

From: "Daniel P. Berrange" <berrange@redhat.com>

Date: Tue, 13 Aug 2013 15:20:43 +0100

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

For https://bugzilla.redhat.com/show_bug.cgi?id=988491

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>

(cherry picked from commit a396473494dd099c2a0e6e03af9a9c7406a86108)

---

 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.2