12:08 p.m.
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
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) {
+ virReportSystemError(errno,
+ _("Cannot find %s - "
+ "Possibly the package isn't installed"),
+ RADVD);
+ goto cleanup;
+ }
+
if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) {
virReportSystemError(err,
_("cannot create directory %s"),
--
1.7.3.4
12:16 p.m.
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.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:55 p.m.
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!
2:08 p.m.
On 03/18/2011 12:55 PM, Laine Stump wrote:
>>
>> + 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).
>
Okay. I pushed it using access(RADVD, X_OK). We can consider converting
everything to virFileIsExecutable separately. Thanks!
Agreed that cleanup should be separate, and also, after more thought,
that we should do it. We already had a case in the past where
access("qemu", X_OK) succeeded on a directory named qemu, but you can't
execute that (in fact, that's why virFileIsExecutable was written!).
Since I'm probably the best at writing cfg.mk rules to catch any
remaining access(.*X_OK), it probably falls to me to write the patch :)
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:46 p.m.
New subject: [libvirt] [PATCH 1/2] util: guarantee sane errno in virFileIsExecutable
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
3:46 p.m.
New subject: [libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)
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).
* 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)) {
ret = 0;
VIR_WARN("Non executable hook script %s", path);
} else {
--
1.7.4
8:43 p.m.
New subject: [libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)
On 03/18/2011 02: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).
I meant to say that access(,R_OK) is not strictly necessary. But I
stand a bit corrected - binaries can be executed without read
permission, but scripts require both execute permission (to do the #!
search) and read permission (for the interpreter to process the script).
Maybe virFileIsExecutable should add a check for readability?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:26 p.m.
New subject: [libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)
On 03/18/2011 09:43 PM, Eric Blake wrote:
On 03/18/2011 02: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).
I meant to say that access(,R_OK) is not strictly necessary. But I
stand a bit corrected - binaries can be executed without read
permission, but scripts require both execute permission (to do the #!
search) and read permission (for the interpreter to process the script).
Maybe virFileIsExecutable should add a check for readability?
I guess yes, but if there's any case where libvirt will want to have X
but not R on a file, it should be optional.
3:28 p.m.
New subject: [libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)
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 {
4:22 p.m.
New subject: [libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)
On 03/24/2011 02:28 PM, Laine Stump wrote:
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).
Thanks - that's a workable plan, especially since it's rare to have a
file with execute but not read permissions in the first place.
> @@ -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".
The message was only a debug level, so I consolidated things to skip it
altogether.
ACK.
Thanks; pushed with this squashed in:
diff --git i/src/util/hooks.c w/src/util/hooks.c
index 149ff21..99dddc4 100644
--- i/src/util/hooks.c
+++ w/src/util/hooks.c
@@ -94,13 +94,12 @@ static int virHooksFound = -1;
static int
virHookCheck(int no, const char *driver) {
char *path;
- struct stat sb;
int ret;
if (driver == NULL) {
virHookReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid hook name for #%d"), no);
- return(-1);
+ return -1;
}
ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, driver);
@@ -108,24 +107,19 @@ virHookCheck(int no, const char *driver) {
virHookReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to build path for %s hook"),
driver);
- return(-1);
+ return -1;
}
- 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);
- } else {
- ret = 1;
- VIR_DEBUG("Found hook script %s", path);
- }
+ ret = 1;
+ VIR_DEBUG("Found hook script %s", path);
}
VIR_FREE(path);
- return(ret);
+ return ret;
}
/*
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:15 p.m.
New subject: [libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:54 a.m.
New subject: [libvirt] [PATCH 1/2] util: guarantee sane errno in virFileIsExecutable
2011/3/18 Eric Blake <eblake(a)redhat.com>:
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.
---
ACK.
Matthias
10:27 a.m.
New subject: [libvirt] [PATCH 1/2] util: guarantee sane errno in virFileIsExecutable
On 03/19/2011 07:54 AM, Matthias Bolte wrote:
2011/3/18 Eric Blake <eblake(a)redhat.com>:
> 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.
> ---
>
ACK.
Thanks; pushed. Did you also get to look at patch 2 in the series,
including the question of whether virFileIsExecutable should be checking
access(file,R_OK|X_OK) for the purpose of executing shell scripts?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4991
days inactive
4998
days old
12 comments
3 participants
participants (3)
-
Eric Blake -
Laine Stump -
Matthias Bolte