Change in vdsm[master]: Fix race in fileUtils.createdir().

zhshzhou at linux.vnet.ibm.com zhshzhou at linux.vnet.ibm.com
Tue Oct 23 09:40:08 UTC 2012


Zhou Zheng Sheng has posted comments on this change.

Change subject: Fix race in fileUtils.createdir().
......................................................................


Patch Set 2: (3 inline comments)

Wow Dan approved very quickly, I attended a meeting just now.

Well my suggestions can be ignored, just about the style.

....................................................
File vdsm/storage/fileUtils.py
Line 172:     Recursively create directory if doesn't exist
Line 173: 
Line 174:     If already exists check that permissions are as requested.
Line 175:     """
Line 176:     DIR_BIT = 040000
How about

  DIR_BIT = stat.S_IFDIR

and if you applied the suggestion of using stat.S_IMODE to get the file permission, DIR_BIT will be not very useful here.
Line 177:     params = (dirPath, mode) if mode is not None else (dirPath,)
Line 178:     try:
Line 179:         os.makedirs(*params)
Line 180:     except OSError as e:


Line 182:             raise
Line 183:         else:
Line 184:             log.warning("Dir %s already exists", dirPath)
Line 185:             if mode is not None:
Line 186:                 statinfo = os.stat(dirPath)
Can I suggest adding a line

if stat.S_ISDIR(statinfo.st_mode) == 0:
    raise OSError(errno.EEXIST, 'Something with the same name exists but not a dir')
Line 187:                 if statinfo[stat.ST_MODE] % DIR_BIT - mode % DIR_BIT != 0:
Line 188:                     raise OSError(errno.EPERM,
Line 189:                     "Existing %s dir permissions %s are not as requested %s" %
Line 190:                     (dirPath, oct(statinfo[stat.ST_MODE]), oct(mode)))


Line 183:         else:
Line 184:             log.warning("Dir %s already exists", dirPath)
Line 185:             if mode is not None:
Line 186:                 statinfo = os.stat(dirPath)
Line 187:                 if statinfo[stat.ST_MODE] % DIR_BIT - mode % DIR_BIT != 0:
Maybe statinfo.st_mode is more "modern".

Can I suggerst

  if stat.S_IMODE(statinfo.st_mode) != mode:
Line 188:                     raise OSError(errno.EPERM,
Line 189:                     "Existing %s dir permissions %s are not as requested %s" %
Line 190:                     (dirPath, oct(statinfo[stat.ST_MODE]), oct(mode)))
Line 191: 


--
To view, visit http://gerrit.ovirt.org/8732
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iac856c219a08fa0303e5da1d04f4f6b8e17e07a3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Shu Ming <shuming at linux.vnet.ibm.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>


More information about the vdsm-patches mailing list