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 {
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list