[libvirt] Re: [PATCH] libvirtd: new config-file option: unix_sock_dir [was Re: adding tests....

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:
> [snip]
>
>> +    if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log",
>> +                 dir_prefix) >= PATH_MAX)
>> +        goto snprintf_error;
>
> If I'm reading correctly, this will cause system logs to get put in
> the directory  /var/.libvirt/log   instead of /var/log/libvirt, since
> this snprintf doesn't take account of uid == SYSTEM_UID as the old
> code used todo.

Good catch.
I'd offer to add a root-only test to make sure the log file is
created as advertised, but would rather first add another config-file
option: to specify where the log file goes.

However, first things first:

Here's a patch that adds two blocks, neither pretty,
but with less duplication than the 3rd alternative,
which duplicates both the snprintf and the result comparison.
(of course, I'll use only one of them)

BTW, this also eliminates the uses of PATH_MAX that were
vestiges of a messy rebase.  Now we test against maxlen.

Of these two, I prefer the latter (slightly less duplication).
Do you care?

diff --git a/qemud/qemud.c b/qemud/qemud.c
index f8c3c97..ddcb6ff 100644
--- a/qemud/qemud.c
+++ b/qemud/qemud.c
@@ -766,8 +766,16 @@ static int qemudInitPaths(struct qemud_server *server,
             goto snprintf_error;
     }

-    if (snprintf(server->logDir, PATH_MAX, "%s/.libvirt/log",
-                 dir_prefix) >= PATH_MAX)
+    if ((uid == SYSTEM_UID
+         ? snprintf(server->logDir, maxlen, "%s/log/libvirt", LOCAL_STATE_DIR)
+         : snprintf(server->logDir, maxlen, "%s/.libvirt/log", dir_prefix))
+        >= maxlen)
+        goto snprintf_error;
+
+    if (snprintf(server->logDir, maxlen,
+                 (uid == SYSTEM_UID ? "%s/log/libvirt" : "%s/.libvirt/log"),
+                 (uid == SYSTEM_UID ? LOCAL_STATE_DIR : dir_prefix))
+        >= maxlen)
         goto snprintf_error;

     ret = 0;