Taught util.misc.mkdir() to set umask to allow group-w for makedirs c… #4000
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,16 +40,20 @@ def mkdir(*args, **kwargs): | |
""" | ||
Create the specified directory. | ||
Tolerant of race conditions. | ||
Sets umask to 002. | ||
|
||
:param args: path[, mode] that goes to os.makedirs | ||
:param kwargs: path | ||
:return: | ||
""" | ||
mask = os.umask(0002) | ||
try: | ||
os.makedirs(*args, **kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The umask is apparently never restored if this raises an exception. Should the original os.umask be above the "try", and the umask reset in a new "finally" block? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...I am an idiot. I had exactly that at some point, and apparently lost it somewhere along the way. Thank you for the great catch, that would have been very embarrassing to allow out into the wild! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
except OSError, e: | ||
if e.errno != errno.EEXIST: | ||
raise | ||
finally: | ||
os.umask(mask) | ||
|
||
|
||
def get_parent_directory(path): | ||
|
@@ -96,9 +100,8 @@ def create_symlink(source_path, link_path, directory_permissions=0770): | |
link_path = link_path[:-1] | ||
|
||
link_parent_dir = os.path.dirname(link_path) | ||
|
||
if not os.path.exists(link_parent_dir): | ||
os.makedirs(link_parent_dir, mode=directory_permissions) | ||
mkdir(link_parent_dir, mode=directory_permissions) | ||
elif os.path.lexists(link_path): | ||
if os.path.islink(link_path): | ||
link_target = os.readlink(link_path) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,8 @@ | |
|
||
from pulp.common.constants import RESOURCE_MANAGER_WORKER_NAME, SCHEDULER_WORKER_NAME | ||
from pulp.common import constants, dateutils, tags | ||
from pulp.plugins.util import misc | ||
|
||
from pulp.server.async.celery_instance import celery, RESOURCE_MANAGER_QUEUE, \ | ||
DEDICATED_QUEUE_EXCHANGE | ||
from pulp.server.exceptions import PulpException, MissingResource, \ | ||
|
@@ -780,11 +782,7 @@ def _handle_cProfile(self, task_id): | |
if config.getboolean('profiling', 'enabled') is True: | ||
self.pr.disable() | ||
profile_directory = config.get('profiling', 'directory') | ||
try: | ||
os.makedirs(profile_directory, 0755) | ||
except OSError as exc: | ||
if exc.errno != errno.EEXIST: | ||
raise | ||
misc.mkdir(profile_directory, mode=0755) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 0775 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this is important and i understand correctly how umask works - if having mode 0755 the result will be: if having mode 0775 the result will be: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left a few things like certificate-directories and this profiling-directory alone - they're not 'content', and are more sensitive to permissions. Unless we're migrating profiling-information, this set of perms shouldn't affect migration, and I felt it better to leave the permissions as they had been specifically set. (Your understanding of umask is correct - it turns off permission-bits where the umask has a 1. So 0755, with the mkdirs() umask of 0002, leaves 0755) |
||
self.pr.dump_stats("%s/%s" % (profile_directory, task_id)) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,22 +6,7 @@ | |
from hashlib import sha256 | ||
|
||
from pulp.server.config import config | ||
|
||
|
||
def mkdir(path): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 let's say no to the code duplication! |
||
""" | ||
Create a directory at the specified path. | ||
Directory (and intermediate) directories are only created if they | ||
don't already exist. | ||
|
||
:param path: The absolute path to the leaf directory to be created. | ||
:type path: str | ||
""" | ||
try: | ||
os.makedirs(path) | ||
except OSError as e: | ||
if e.errno != errno.EEXIST: | ||
raise | ||
from pulp.plugins.util import misc | ||
|
||
|
||
class ContentStorage(object): | ||
|
@@ -124,7 +109,7 @@ def put(self, unit, path, location=None): | |
destination = unit.storage_path | ||
if location: | ||
destination = os.path.join(destination, location.lstrip('/')) | ||
mkdir(os.path.dirname(destination)) | ||
misc.mkdir(os.path.dirname(destination)) | ||
fd, temp_destination = tempfile.mkstemp(dir=os.path.dirname(destination)) | ||
|
||
# to avoid a file descriptor leak, close the one opened by tempfile.mkstemp which we are not | ||
|
@@ -212,8 +197,8 @@ def open(self): | |
Open the shared storage. | ||
The shared storage location is created as needed. | ||
""" | ||
mkdir(self.content_dir) | ||
mkdir(self.links_dir) | ||
misc.mkdir(self.content_dir) | ||
misc.mkdir(self.links_dir) | ||
|
||
@property | ||
def shared_dir(self): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to check for presence of the path here, remove the if condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The os.access() check in the elif is only needed if we're not creating the path in the mkdir() call. I tried a couple of iterations here, and decided that while this one is a little redundant around the mkdir(), it seemed more clear in terms of understanding the code. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooh right, i did not notice the elif statement, i am cool with leaving if