Merge lp:~andreserl/maas/maas_set_correct_file_permissions into lp:~maas-committers/maas/trunk

[Julian Edwards]

[Julian Edwards (julian-edwards)]

This is great! But it needs tests...

review: Needs Fixing

[Julian Edwards (julian-edwards)]

BTW I took the liberty of re-proposing the merge so it has the pre-requisite branch set, which makes the diff only show what this branch changes.

[Jeroen T. Vermeulen (jtv)]

In src/provisioningserver/pxe/install_image.py, the chmod is unnecessarily being done in the critical section that we want to keep as brief as possible. Do it before, not during!

In src/provisioningserver/utils.py, the comment says we "assume" that 'filename' is not a file -- right after the "if not os.is_file(filename) that actually proves it. Can you find a better way to word that?

[Jeroen T. Vermeulen (jtv)]

Ah, I rubbed the sleep out of my eyes and took a better look. I was completely mistaken about the second issue -- although I still can't figure out what the basis for the assumption is. I suspect that chmod inside atomic_write may belong in the caller.

[Jeroen T. Vermeulen (jtv)]

Nope, caller is no good for this. It can't be done before or after atomic_write. But the chmod is in the wrong place: it effectively makes the write non-atomic by creating a race window where the file may be non-accessible for no good reason.

I still can't make sense of the comment... Clearly "filename" is not a directory. It is the file you just wrote.

[Jeroen T. Vermeulen (jtv)]

Anyway, as per the kanban board I already had a branch in progress for this. I'll move that forward; it's a bit simpler. I also marked the bug as in-progress and assigned to myself, to match the board.

=== modified file 'src/provisioningserver/pxe/install_image.py'
--- src/provisioningserver/pxe/install_image.py	2012-07-23 10:24:39 +0000
+++ src/provisioningserver/pxe/install_image.py	2012-08-30 02:02:23 +0000
@@ -108,6 +108,7 @@
     if os.path.isdir(old):
         os.rename(old, '%s.old' % old)
     os.rename('%s.new' % old, old)
+    os.chmod(old, 0755)
     # End of critical window.
 
     # Now delete the old image directory at leisure.
 
=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-08-24 09:21:34 +0000
+++ src/provisioningserver/utils.py	2012-08-30 02:02:23 +0000
@@ -75,6 +75,9 @@
             if not os.path.isfile(filename):
                 temp_file = _write_temp_file(content, filename)
                 os.rename(temp_file, filename)
+                # Setting 0755 as we assume `filename` is not a file
+                # but rather it is a directory.
+                os.chmod(filename, 0755)
         finally:
             # Release the lock.
             lock.release()
@@ -83,6 +86,7 @@
         # POSIX systems.
         temp_file = _write_temp_file(content, filename)
         os.rename(temp_file, filename)
+        os.chmod(filename, 0644)
 
 
 def incremental_write(content, filename):