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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list