Re: [libvirt] [PATCH] libvirtd: create run dir when running at non-root user

2 Sep 2011

On 08/28/2011 08:52 PM, xuhj@linux.vnet.ibm.com wrote:
...
From: Xu He Jie<xuhj@linux.vnet.ibm.com>
Signed-off-by: Xu He Jie<xuhj@linux.vnet.ibm.com>
Usually this line goes on the bottom, not the top, of a commit message.
...
When libvirtd is running at non-root user, it won't create ${HOME}/.libvirt.
It will show error message:
17:44:16.838: 7035: error : virPidFileAcquirePath:322 : Failed to open pid file
+        run_dir = strdup(LOCALSTATEDIR "/run/libvirt");
+        if (!run_dir) {
+            VIR_ERROR(_("Can't allocate memory"));
virReportOOMError(), not a hand-rolled error reporting.
...
+            goto cleanup;
+        }
+    }
+    else {
Style: '} else {' with no newline.
...
+        char *user_dir = NULL;
+
+        if (!(user_dir = virGetUserDirectory(geteuid()))) {
+            VIR_ERROR(_("Can't determine user directory"));
+            goto cleanup;
+        }
+
+        if (virAsprintf(&run_dir, "%s/.libvirt/", user_dir)<  0) {
+            VIR_ERROR(_("Can't allocate memory"));
We can consolidate OOM checking for both branches by sinking it below 
the if.
...
+            VIR_FREE(user_dir);
+            goto cleanup;
          }
-        umask(old_umask);
Hmm, on second thought, virFileMakePath is documented as explicitly 
using 0777 - umask, but we really do want the new directory to be 0755 
if we create it.  I think I'll keep the umask modifications intact (if 
someone can convince me otherwise, especially if we have a use case for 
0775 instead of 0777 or 0755, we can change that in a later patch).
...
+    if (run_dir)
+        VIR_FREE(run_dir);
Fails 'make syntax-check', due to a useless if before free.

ACK with these changes squashed in, so I pushed.  I also added you to 
AUTHORS; let me know if I need to update anything there.

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index e0004c7..7734fff 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1250,6 +1250,7 @@ int main(int argc, char **argv) {
      bool implicit_conf = false;
      bool use_polkit_dbus;
      char *run_dir = NULL;
+    mode_t old_umask;

      struct option opts[] = {
          { "verbose", no_argument, &verbose, 1},
@@ -1405,28 +1406,22 @@ int main(int argc, char **argv) {
      /* Ensure the rundir exists (on tmpfs on some systems) */
      if (privileged) {
          run_dir = strdup(LOCALSTATEDIR "/run/libvirt");
-        if (!run_dir) {
-            VIR_ERROR(_("Can't allocate memory"));
-            goto cleanup;
-        }
-    }
-    else {
-        char *user_dir = NULL;
+    } else {
+        char *user_dir = virGetUserDirectory(geteuid());

-        if (!(user_dir = virGetUserDirectory(geteuid()))) {
+        if (!user_dir) {
              VIR_ERROR(_("Can't determine user directory"));
              goto cleanup;
          }
-
-        if (virAsprintf(&run_dir, "%s/.libvirt/", user_dir) < 0) {
-            VIR_ERROR(_("Can't allocate memory"));
-            VIR_FREE(user_dir);
-            goto cleanup;
-        }
-
+        ignore_value(virAsprintf(&run_dir, "%s/.libvirt/", user_dir));
          VIR_FREE(user_dir);
      }
+    if (!run_dir) {
+        virReportOOMError();
+        goto cleanup;
+    }

+    old_umask = umask(022);
      if (virFileMakePath(run_dir) < 0) {
          char ebuf[1024];
          VIR_ERROR(_("unable to create rundir %s: %s"), run_dir,
@@ -1434,6 +1429,7 @@ int main(int argc, char **argv) {
          ret = VIR_DAEMON_ERR_RUNDIR;
          goto cleanup;
      }
+    umask(old_umask);

      /* Try to claim the pidfile, exiting if we can't */
      if ((pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0) {
@@ -1586,8 +1582,7 @@ cleanup:
      VIR_FREE(sock_file_ro);
      VIR_FREE(pid_file);
      VIR_FREE(remote_config_file);
-    if (run_dir)
-        VIR_FREE(run_dir);
+    VIR_FREE(run_dir);

      daemonConfigFree(config);
      virLogShutdown();


-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

    