"Daniel P. Berrange" <berrange@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;