[NEW PATCH] BZ#XXXXXX Use only os.access to check permissions (via gerrit-bot)

[Federico Simoncelli]

New patch submitted by Federico Simoncelli (fsimonce(a)redhat.com)

You can review this change at: http://gerrit.usersys.redhat.com/733

commit 296dd556ea6cb51301a305971e2f5a1566d9f93a
Author: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Date:   Fri Jul 22 14:48:56 2011 +0000

    BZ#XXXXXX Use only os.access to check permissions
    
    Change-Id: Ic7b892886416e866178ac88c04cbfb68ed0c055b

diff --git a/vdsm/storage/fileUtils.py b/vdsm/storage/fileUtils.py
index adbbf4b..c76e097 100644
--- a/vdsm/storage/fileUtils.py
+++ b/vdsm/storage/fileUtils.py
@@ -182,24 +182,20 @@ def validatePermissions(targetPath):
     if st.st_uid != uid or st.st_gid != gid:
         raise se.StorageServerAccessPermissionError(targetPath)
 
-def pathExists(filename, writeable=False):
+def pathExists(filename, writable=False):
     # This function is workarround for a NFS issue where
     # sometimes os.exists/os.access fails due to NFS stale handle.
-    # In such cases we should try again and stat the file
+    try:
+        os.stat(filename)
+    except OSError:
+        return False
+
     check = os.R_OK
-    if writeable:
-        check |= os.W_OK
 
-    if os.access(filename, check):
-        return True
+    if writable:
+        check |= os.W_OK
 
-    try:
-        s = os.stat(filename)
-        if check & s[0] == check:
-            return True
-    except OSError:
-        pass
-    return False
+    return os.access(filename, check)
 
 def cleanupfiles(filelist):
     """

Change in vdsm[master]: BZ#726630 Use only os.access to check permissions

Jakub Libosvar has posted comments on this change.

Change subject: BZ#726630 Use only os.access to check permissions
......................................................................


Patch Set 2: Verified

--
To view, visit http://gerrit.usersys.redhat.com/733
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7b892886416e866178ac88c04cbfb68ed0c055b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Jakub Libosvar <libosvar(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#726630 Use only os.access to check permissions

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#726630 Use only os.access to check permissions
......................................................................


BZ#726630 Use only os.access to check permissions

The old implementation of fileUtils.pathExists was using a flawed
re-implementation of os.access as backup check for files with a NFS
stale handle, the consequence was that accessible files were
reported as non-accessible.
We now use os.stat to refresh the NFS handle and then os.access to
determine weather we can read and write the file.

Change-Id: Ic7b892886416e866178ac88c04cbfb68ed0c055b
---
M vdsm/storage/fileUtils.py
1 file changed, 9 insertions(+), 13 deletions(-)

Approvals:
  Dan Kenigsberg: Looks good to me, approved
  Jakub Libosvar: Verified


--
To view, visit http://gerrit.usersys.redhat.com/733
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic7b892886416e866178ac88c04cbfb68ed0c055b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Jakub Libosvar <libosvar(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#726630 Use only os.access to check permissions

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#726630 Use only os.access to check permissions
......................................................................


Patch Set 2: Looks good to me, approved

Ok, the point that I was missing is that comparing to os.R_OK only caught stat.S_IROTH.
stat should compare only against stat bits...

--
To view, visit http://gerrit.usersys.redhat.com/733
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7b892886416e866178ac88c04cbfb68ed0c055b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Jakub Libosvar <libosvar(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#XXXXXX Use only os.access to check permissions

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#XXXXXX Use only os.access to check permissions
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

Your code seems find, but I do not understand the specific problem with the replaced code.
Could you elaborate on this in the commit message?

....................................................
Commit Message
Line 7: BZ#XXXXXX Use only os.access to check permissions
BZ#726630 (I know, you where quicker than the reporter)

--
To view, visit http://gerrit.usersys.redhat.com/733
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7b892886416e866178ac88c04cbfb68ed0c055b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

Change in vdsm[master]: BZ#XXXXXX Use only os.access to check permissions

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: BZ#XXXXXX Use only os.access to check permissions
......................................................................


Patch Set 1: (3 inline comments)

....................................................
File vdsm/storage/fileUtils.py
Line 185: def pathExists(filename, writable=False):
I already checked that nobody was relaying on this typo.

Line 189:         os.stat(filename)
This should be enough to circumvent the nfs stale handle. If anyone is concerned with the
fact that we are always running os.stat, the other solution would be:

 if os.access(filename, check):
     return True

 try:
     os.stat(filename)
 except OSError:
     return False
 return os.access(filename, check)

I'm not sure if this is where we want to optimize. Consider also that when the first
os.access return False we're going to do more operations.

Line 197
Re-implementing os.access is not easy. First of all we should check against the correct
constants: S_IRUSR, S_IWUSR, S_IRGRP, S_IWGRP, etc.
The value R_OK just happened to work in some rare cases.
Second, os.access does much more, it checks the process uid and gid against the file
owner/group, checks if the mount is read-only and a bunch of other things.

--
To view, visit http://gerrit.usersys.redhat.com/733
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7b892886416e866178ac88c04cbfb68ed0c055b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;

Change in vdsm[master]: BZ#XXXXXX Use only os.access to check permissions

Saggi Mizrahi has posted comments on this change.

Change subject: BZ#XXXXXX Use only os.access to check permissions
......................................................................


Patch Set 1: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.usersys.redhat.com/733
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7b892886416e866178ac88c04cbfb68ed0c055b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;