[libvirt] [PATCH 2/2] maint: prohibit access(,X_OK)

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110324/2eaa912d/attachment-0001.sig>