On 03/18/2011 01:16 PM, Eric Blake wrote:

On 03/18/2011 11:08 AM, Laine Stump wrote:

...

This is detailed in:

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

Since radvd is executed by daemonizing it, the attempt to exec the radvd binary doesn't happen until after libvirtd has already received an exit code from the intermediate forked process, so no error is detected or logged by __virExec().

We can't require radvd as a prerequisit for the libvirt package (many s/prerequisit/prerequisite/

...

installations don't use IPv6, so they don't need it), so instead we add in a check to verify there is an executable radvd binary prior to trying to exec it. --- src/network/bridge_driver.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6a02df1..c30620a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -689,6 +689,14 @@ networkStartRadvd(virNetworkObjPtr network)

network->radvdPid = -1;

+ if (access(RADVD, X_OK)< 0) { Hmm, should this use

if (!virFileIsExecutable(RADVD)) {

instead? Then again, I count 10 instances of 'access.*X_OK', but only 5 of FileIsExecutable, so maybe it's worth a separate cleanup (and a cfg.mk rule to enforce whatever decision we make).

...

+ virReportSystemError(errno, + _("Cannot find %s - " + "Possibly the package isn't installed"), + RADVD); + goto cleanup; + } ACK with the commit typo fixed.

Okay. I pushed it using access(RADVD, X_OK). We can consider converting everything to virFileIsExecutable separately. Thanks!

If virFileIsExecutable is to replace access(file,X_OK), then errno must be usable on failure. * src/util/util.c (virFileIsExecutable): Set errno on failure. --- src/util/util.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 5b5dd5e..1e4e2ab 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1357,7 +1357,8 @@ bool virFileExists(const char *path) return access(path, F_OK) == 0; } -/* Check that a file is regular and has executable bits. +/* Check that a file is regular and has executable bits. If false is + * returned, errno is valid. * * Note: In the presence of ACLs, this may return true for a file that * would actually fail with EACCES for a given user, or false for a @@ -1370,9 +1371,12 @@ virFileIsExecutable(const char *file) /* We would also want to check faccessat if we cared about ACLs, * but we don't. */ - return (stat(file, &sb) == 0 && - S_ISREG(sb.st_mode) && - (sb.st_mode & 0111) != 0); + if (stat(file, &sb) < 0) + return false; + if (S_ISREG(sb.st_mode) && (sb.st_mode & 0111) != 0) + return true; + errno = S_ISDIR(sb.st_mode) ? EISDIR : EACCES; + return false; } #ifndef WIN32 -- 1.7.4

I noticed you hadn't committed this yet, so I came back to take a look... On 03/18/2011 04:46 PM, Eric Blake wrote:

This simplifies several callers that were repeating checks already guaranteed by util.c, and makes other callers more robust to now reject directories. remote_driver.c was over-strict - access(,X_OK) is not strictly needed to execute a file (although its unusual to see a file with X_OK but not R_OK).

In your followup message you wondered whether virFileIsExecutable should check for readability. I guess if we want to allow people to replace the commands with shell scripts, then we can do one of two things: 1) check for readability. Or, 2) just warn people that if they're using a shell script, they need to make sure it is readable. if they're already hacking around that much, this is probably a small thing to ask, so I think until somebody complains it can stay as it is (not checking readability), since that's the way it already is in all but one case (and that one is probably the *least* likely to be replaced with a shell script).

* cfg.mk (sc_prohibit_access_xok): New syntax-check rule. (exclude_file_name_regexp--sc_prohibit_access_xok): Exempt one use. * src/network/bridge_driver.c (networkStartRadvd): Fix offenders. * src/qemu/qemu_capabilities.c (qemuCapsProbeMachineTypes) (qemuCapsInitGuest, qemuCapsInit, qemuCapsExtractVersionInfo): Likewise. * src/remote/remote_driver.c (remoteFindDaemonPath): Likewise. * src/uml/uml_driver.c (umlStartVMDaemon): Likewise. * src/util/hooks.c (virHookCheck): Likewise. ---

This patch depends on the unreviewed: https://www.redhat.com/archives/libvir-list/2011-March/msg00770.html

cfg.mk | 9 +++++++++ src/network/bridge_driver.c | 2 +- src/qemu/qemu_capabilities.c | 15 +++++---------- src/remote/remote_driver.c | 2 +- src/uml/uml_driver.c | 2 +- src/util/hooks.c | 4 ++-- 6 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/cfg.mk b/cfg.mk index 2a97f88..ac419f7 100644 --- a/cfg.mk +++ b/cfg.mk @@ -258,6 +258,13 @@ sc_prohibit_fork_wrappers: halt='use virCommand for child processes' \ $(_sc_search_regexp)

+# access with X_OK accepts directories, but we can't exec() those. +# access with F_OK or R_OK is okay, though. +sc_prohibit_access_xok: + @prohibit='access''(at)? *\(.*X_OK' \ + halt='use virFileIsExecutable instead of access''(,X_OK)' \ + $(_sc_search_regexp) + # Similar to the gnulib maint.mk rule for sc_prohibit_strcmp # Use STREQLEN or STRPREFIX rather than comparing strncmp == 0, or != 0. sc_prohibit_strncmp: @@ -551,6 +558,8 @@ exclude_file_name_regexp--sc_po_check = ^docs/ exclude_file_name_regexp--sc_prohibit_VIR_ERR_NO_MEMORY = \ ^(include/libvirt/virterror\.h|daemon/dispatch\.c|src/util/virterror\.c)$$

+exclude_file_name_regexp--sc_prohibit_access_xok = ^src/util/util\.c$$ + exclude_file_name_regexp--sc_prohibit_always_true_header_tests = \ (^docs|^python/(libvirt-override|typewrappers)\.c$$)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c30620a..fb933a2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -689,7 +689,7 @@ networkStartRadvd(virNetworkObjPtr network)

network->radvdPid = -1;

- if (access(RADVD, X_OK)< 0) { + if (!virFileIsExecutable(RADVD)) { virReportSystemError(errno, _("Cannot find %s - " "Possibly the package isn't installed"), diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d8aa9cd..f86e7f5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -178,7 +178,7 @@ qemuCapsProbeMachineTypes(const char *binary, * Technically we could catch the exec() failure, but that's * in a sub-process so it's hard to feed back a useful error. */ - if (access(binary, X_OK)< 0) { + if (!virFileIsExecutable(binary)) { virReportSystemError(errno, _("Cannot find QEMU binary %s"), binary); return -1; } @@ -451,12 +451,9 @@ qemuCapsInitGuest(virCapsPtr caps, */ binary = virFindFileInPath(info->binary);

- if (binary == NULL || access(binary, X_OK) != 0) { + if (binary == NULL || !virFileIsExecutable(binary)) { VIR_FREE(binary); binary = virFindFileInPath(info->altbinary); - - if (binary != NULL&& access(binary, X_OK) != 0) - VIR_FREE(binary); }

/* Can use acceleration for KVM/KQEMU if @@ -475,10 +472,8 @@ qemuCapsInitGuest(virCapsPtr caps, for (i = 0; i< ARRAY_CARDINALITY(kvmbins); ++i) { kvmbin = virFindFileInPath(kvmbins[i]);

- if (kvmbin == NULL || access(kvmbin, X_OK) != 0) { - VIR_FREE(kvmbin); + if (!kvmbin) continue; - }

haskvm = 1; if (!binary) @@ -753,7 +748,7 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps) /* Then possibly the Xen paravirt guests (ie Xenner */ xenner = virFindFileInPath("xenner");

- if (xenner != NULL&& access(xenner, X_OK) == 0&& + if (xenner != NULL&& virFileIsExecutable(xenner) == 0&& access("/dev/kvm", F_OK) == 0) { for (i = 0 ; i< ARRAY_CARDINALITY(arch_info_xen) ; i++) /* Allow Xen 32-on-32, 32-on-64 and 64-on-64 */ @@ -1147,7 +1142,7 @@ int qemuCapsExtractVersionInfo(const char *qemu, const char *arch, * Technically we could catch the exec() failure, but that's * in a sub-process so it's hard to feed back a useful error. */ - if (access(qemu, X_OK)< 0) { + if (!virFileIsExecutable(qemu)) { virReportSystemError(errno, _("Cannot find QEMU binary %s"), qemu); return -1; } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b844d9a..a8fb52d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -306,7 +306,7 @@ remoteFindDaemonPath(void) return(customDaemon);

for (i = 0; serverPaths[i]; i++) { - if (access(serverPaths[i], X_OK | R_OK) == 0) { + if (virFileIsExecutable(serverPaths[i])) { return serverPaths[i]; } } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 538d5f7..2af53ff 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -829,7 +829,7 @@ static int umlStartVMDaemon(virConnectPtr conn, * Technically we could catch the exec() failure, but that's * in a sub-process so its hard to feed back a useful error */ - if (access(vm->def->os.kernel, X_OK)< 0) { + if (!virFileIsExecutable(vm->def->os.kernel)) { virReportSystemError(errno, _("Cannot find UML kernel %s"), vm->def->os.kernel); diff --git a/src/util/hooks.c b/src/util/hooks.c index 5ba2036..8231349 100644 --- a/src/util/hooks.c +++ b/src/util/hooks.c @@ -1,7 +1,7 @@ /* * hooks.c: implementation of the synchronous hooks support * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2010 Daniel Veillard * * This library is free software; you can redistribute it and/or @@ -113,7 +113,7 @@ virHookCheck(int no, const char *driver) { ret = 0; VIR_DEBUG("No hook script %s", path); } else { - if ((access(path, X_OK) != 0) || (!S_ISREG(sb.st_mode))) { + if (!virFileIsExecutable(path)) {

Here you (rightly) removed the examination of sb. That means that the only result of the call to stat() (just out of view of the diff) used is the return value. Maybe that could be changed to virFileExists(), or even just simplify the code and print the same message for "doesn't exist" and "isn't executable". I'm fine leaving it as is too. ACK.

ret = 0; VIR_WARN("Non executable hook script %s", path); } else {

On 03/24/2011 03:22 PM, Eric Blake wrote:

...

Here you (rightly) removed the examination of sb. That means that the only result of the call to stat() (just out of view of the diff) used is the return value. Maybe that could be changed to virFileExists(), or even just simplify the code and print the same message for "doesn't exist" and "isn't executable".

The message was only a debug level, so I consolidated things to skip it altogether.

- if (stat(path, &sb) < 0) { + if (!virFileIsExecutable(path)) { ret = 0; - VIR_DEBUG("No hook script %s", path); + VIR_WARN("Missing or non-executable hook script %s", path); } else { - if (!virFileIsExecutable(path)) { - ret = 0; - VIR_WARN("Non executable hook script %s", path);

Aargh, I didn't do it right. It was debug on missing but warn on non-executable; now it's always warn (which means missing is noisier than it used to be). I'm applying this as an obvious followup to fix that minor regression. From fd79f2d51e0de224a5389ddfa8c24c6cc0d3ab4d Mon Sep 17 00:00:00 2001 From: Eric Blake <eblake@redhat.com> Date: Fri, 25 Mar 2011 15:14:27 -0600 Subject: [PATCH] hooks: fix regression in previous patch * src/util/hooks.c (virHookCheck): Missing hooks should just be debug, not warn. --- src/util/hooks.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/util/hooks.c b/src/util/hooks.c index 99dddc4..41598a6 100644 --- a/src/util/hooks.c +++ b/src/util/hooks.c @@ -110,9 +110,12 @@ virHookCheck(int no, const char *driver) { return -1; } - if (!virFileIsExecutable(path)) { + if (!virFileExists(path)) { ret = 0; - VIR_WARN("Missing or non-executable hook script %s", path); + VIR_DEBUG("No hook script %s", path); + } else if (!virFileIsExecutable(path)) { + ret = 0; + VIR_WARN("Non-executable hook script %s", path); } else { ret = 1; VIR_DEBUG("Found hook script %s", path); -- 1.7.4 -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org