[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:

> On Mon, Feb 09, 2009 at 02:01:19PM +0100, Jim Meyering wrote:
>> 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?
>
> I don't particular like either option - too much line noise in there.
> Two thoughts - printf is redundant in the first place, since the compiler
> will happily concatenate 2 static strings, so we can just strdup(). In
> the second case, we could asprintf instead, and so have something like
>
>   if (uid == SYSTEM_UID)
>      server->logDir = strdup(LOCAL_STATE_DIR "/log/libvirt")
>   else
>      virAsprintf(&server->logDir, "%s/.libvirt/log", dir_prefix))
>
>   if (!server->logDir)
>      ... oom handling ...

Can't do that.  The logDir member is declared like this:

  qemud/qemud.h:    char logDir[PATH_MAX];

which made me see why it was using PATH_MAX.
and we always want to check for buffer overrun.
so here's what I'll do:

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

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

     ret = 0;