Merge branch 'file_fixes_clean' of https://github.com/bcoca/ansible into bcoca-file_fixes_clean

lib/ansible/module_utils/basic.py

@@ -464,7 +464,7 @@ class AnsibleModule(object):
                 changed = True
         return changed
 
-    def set_file_attributes_if_different(self, file_args, changed):
+    def set_fs_attributes_if_different(self, file_args, changed):
         # set modes owners and context as needed
         changed = self.set_context_if_different(
             file_args['path'], file_args['secontext'], changed
@@ -481,19 +481,10 @@ class AnsibleModule(object):
         return changed
 
     def set_directory_attributes_if_different(self, file_args, changed):
-        changed = self.set_context_if_different(
-            file_args['path'], file_args['secontext'], changed
-        )
-        changed = self.set_owner_if_different(
-            file_args['path'], file_args['owner'], changed
-        )
-        changed = self.set_group_if_different(
-            file_args['path'], file_args['group'], changed
-        )
-        changed = self.set_mode_if_different(
-            file_args['path'], file_args['mode'], changed
-        )
-        return changed
+        return self.set_fs_attributes_if_different(file_args, changed)
+
+    def set_file_attributes_if_different(self, file_args, changed):
+        return self.set_fs_attributes_if_different(file_args, changed)
 
     def add_path_info(self, kwargs):
         '''
@@ -963,7 +954,7 @@ class AnsibleModule(object):
                 context = self.selinux_default_context(dest)
 
         try:
-            # Optimistically try a rename, solves some corner cases and can avoid useless work.
+            # Optimistically try a rename, solves some corner cases and can avoid useless work, throws exception if not atomic.
             os.rename(src, dest)
         except (IOError,OSError), e:
             # only try workarounds for errno 18 (cross device), 1 (not permited) and 13 (permission denied)

library/files/file

@@ -139,9 +139,6 @@ EXAMPLES = '''
 
 def main():
 
-    # FIXME: pass this around, should not use global
-    global module
-
     module = AnsibleModule(
         argument_spec = dict(
             state = dict(choices=['file','directory','link','hard','touch','absent'], default=None),
@@ -151,6 +148,7 @@ def main():
             force = dict(required=False,default=False,type='bool'),
             diff_peek = dict(default=None),
             validate = dict(required=False, default=None),
+            src = dict(required=False, default=None),
         ),
         add_file_common_args=True,
         supports_check_mode=True
@@ -159,10 +157,14 @@ def main():
     params = module.params
     state  = params['state']
     force = params['force']
+    diff_peek = params['diff_peek']
+    src = params['src']
+
+    # modify source as we later reload and pass, specially relevant when used by other modules.
     params['path'] = path = os.path.expanduser(params['path'])
 
     # short-circuit for diff_peek
-    if params.get('diff_peek', None) is not None:
+    if diff_peek is not None:
         appears_binary = False
         try:
             f = open(path)
@@ -174,8 +176,8 @@ def main():
             pass
         module.exit_json(path=path, changed=False, appears_binary=appears_binary)
 
+    # Find out current state
     prev_state = 'absent'
-
     if os.path.lexists(path):
         if os.path.islink(path):
             prev_state = 'link'
@@ -187,76 +189,60 @@ def main():
             # could be many other things, but defaulting to file
             prev_state = 'file'
 
-    if prev_state is not None and state is None:
-        # set state to current type of file
-        state = prev_state
-    elif state is None:
-        # set default state to file
-        state = 'file'
+    # state should default to file, but since that creates many conflicts,
+    # default to 'current' when it exists.
+    if state is None:
+        if prev_state != 'absent':
+            state = prev_state
+        else:
+            state = 'file'
 
     # source is both the source of a symlink or an informational passing of the src for a template module
     # or copy module, even if this module never uses it, it is needed to key off some things
-
-    src = params.get('src', None)
-    if src:
+    if src is not None:
         src = os.path.expanduser(src)
 
-    if src is not None and os.path.isdir(path) and state not in ["link", "absent"]:
-        if params['original_basename']:
-            basename = params['original_basename']
-        else:
-            basename = os.path.basename(src)
-        params['path'] = path = os.path.join(path, basename)
+        # original_basename is used by other modules that depend on file.
+        if os.path.isdir(path) and state not in ["link", "absent", "directory"]:
+            if params['original_basename']:
+                basename = params['original_basename']
+            else:
+                basename = os.path.basename(src)
+            params['path'] = path = os.path.join(path, basename)
+    else:
+        if state in ['link','hard']:
+            module.fail_json(msg='src and dest are required for creating links')
 
     file_args = module.load_file_common_arguments(params)
-
-    if state in ['link','hard'] and (src is None or path is None):
-        module.fail_json(msg='src and dest are required for creating links')
-    elif path is None:
-        module.fail_json(msg='path is required')
-
     changed = False
 
     recurse = params['recurse']
+    if recurse and state != 'directory':
+        module.fail_json(path=path, msg="recurse option requires state to be 'directory'")
 
-    if recurse and state == 'file' and prev_state == 'directory':
-        state = 'directory'
-
-    if prev_state != 'absent' and state == 'absent':
-        try:
-            if prev_state == 'directory':
-                if os.path.islink(path):
-                    if module.check_mode:
-                        module.exit_json(changed=True)
-                    os.unlink(path)
-                else:
+    if state == 'absent':
+        if state != prev_state:
+            if not module.check_mode:
+                if prev_state == 'directory':
                     try:
-                        if module.check_mode:
-                            module.exit_json(changed=True)
                         shutil.rmtree(path, ignore_errors=False)
                     except Exception, e:
                         module.fail_json(msg="rmtree failed: %s" % str(e))
-            else:
-                if module.check_mode:
-                    module.exit_json(changed=True)
-                os.unlink(path)
-        except Exception, e:
-            module.fail_json(path=path, msg=str(e))
-        module.exit_json(path=path, changed=True)
-
-    if prev_state != 'absent' and prev_state != state:
-        if not (force and (prev_state == 'file' or prev_state == 'hard' or prev_state == 'directory') and state == 'link') and state != 'touch':
-            module.fail_json(path=path, msg='refusing to convert between %s and %s for %s' % (prev_state, state, src))
-
-    if prev_state == 'absent' and state == 'absent':
-        module.exit_json(path=path, changed=False)
-
-    if state == 'file':
+                else:
+                    try:
+                        os.unlink(path)
+                    except Exception, e:
+                        module.fail_json(path=path, msg="unlinking failed: %s " % str(e))
+            module.exit_json(path=path, changed=True)
+        else:
+            module.exit_json(path=path, changed=False)
 
-        if prev_state != 'file':
-            module.fail_json(path=path, msg='file (%s) does not exist, use copy or template module to create' % path)
+    elif state == 'file':
+        if state != prev_state:
+            # file is not absent and any other state is a conflict
+            module.fail_json(path=path, msg='file (%s) is %s, cannot continue' % (path, prev_state))
 
-        changed = module.set_file_attributes_if_different(file_args, changed)
+        changed = module.set_fs_attributes_if_different(file_args, changed)
         module.exit_json(path=path, changed=changed)
 
     elif state == 'directory':
@@ -266,31 +252,29 @@ def main():
             os.makedirs(path)
             changed = True
 
-        changed = module.set_directory_attributes_if_different(file_args, changed)
+        changed = module.set_fs_attributes_if_different(file_args, changed)
+
         if recurse:
             for root,dirs,files in os.walk( file_args['path'] ):
-                for dir in dirs:
-                    dirname=os.path.join(root,dir)
+                for fsobj in dirs + files:
+                    fsname=os.path.join(root, fsobj)
                     tmp_file_args = file_args.copy()
-                    tmp_file_args['path']=dirname
-                    changed = module.set_directory_attributes_if_different(tmp_file_args, changed)
-                for file in files:
-                    filename=os.path.join(root,file)
-                    tmp_file_args = file_args.copy()
-                    tmp_file_args['path']=filename
-                    changed = module.set_file_attributes_if_different(tmp_file_args, changed)
+                    tmp_file_args['path']=fsname
+                    changed = module.set_fs_attributes_if_different(tmp_file_args, changed)
+
         module.exit_json(path=path, changed=changed)
 
     elif state in ['link','hard']:
 
+        if not os.path.exists(src) and not force:
+            module.fail_json(path=path, src=src, msg='src file does not exist')
+
         if state == 'hard':
-            if os.path.isabs(src):
-                abs_src = src
-            else:
+            if not os.path.isabs(src):
                 module.fail_json(msg="absolute paths are required")
 
-            if not os.path.exists(abs_src) and not force:
-                module.fail_json(path=path, src=src, msg='src file does not exist')
+        elif prev_state in ['file', 'hard', 'directory'] and not force:
+            module.fail_json(path=path, msg='refusing to convert between %s and %s for %s' % (prev_state, state, src))
 
         if prev_state == 'absent':
             changed = True
@@ -300,26 +284,29 @@ def main():
                 changed = True
         elif prev_state == 'hard':
             if not (state == 'hard' and os.stat(path).st_ino == os.stat(src).st_ino):
+                changed = True
                 if not force:
                     module.fail_json(dest=path, src=src, msg='Cannot link, different hard link exists at destination')
-                changed = True
-        elif prev_state == 'file':
-            if not force:
-                module.fail_json(dest=path, src=src, msg='Cannot link, file exists at destination')
+        elif prev_state in ['file', 'directory']:
             changed = True
-        elif prev_state == 'directory':
             if not force:
-                module.fail_json(dest=path, src=src, msg='Cannot link, directory exists at destination')
-            changed = True
+                module.fail_json(dest=path, src=src, msg='Cannot link, %s exists at destination' % prev_state)
         else:
             module.fail_json(dest=path, src=src, msg='unexpected position reached')
 
         if changed and not module.check_mode:
             if prev_state != 'absent':
+                # try to replace atomically
+                tmppath = ".%s.%s.%s.tmp" % (path,os.getpid(),time.time())
                 try:
-                    os.unlink(path)
+                    if state == 'hard':
+                        os.link(src,tmppath)
+                    else:
+                        os.symlink(src, tmppath)
+                    os.rename(tmppath, path)
                 except OSError, e:
-                    module.fail_json(path=path, msg='Error while removing existing target: %s' % str(e))
+                    os.unlink(tmppath)
+                    module.fail_json(path=path, msg='Error while replacing: %s' % str(e))
             try:
                 if state == 'hard':
                     os.link(src,path)
@@ -328,30 +315,30 @@ def main():
             except OSError, e:
                 module.fail_json(path=path, msg='Error while linking: %s' % str(e))
 
-        changed = module.set_file_attributes_if_different(file_args, changed)
+        changed = module.set_fs_attributes_if_different(file_args, changed)
         module.exit_json(dest=path, src=src, changed=changed)
 
     elif state == 'touch':
-        if module.check_mode:
-            module.exit_json(path=path, skipped=True)
+        if not module.check_mode:
+
+            if prev_state == 'absent':
+                try:
+                    open(path, 'w').close()
+                except OSError, e:
+                    module.fail_json(path=path, msg='Error, could not touch target: %s' % str(e))
+            elif prev_state in ['file', 'directory']:
+                try:
+                    os.utime(path, None)
+                except OSError, e:
+                    module.fail_json(path=path, msg='Error while touching existing target: %s' % str(e))
+            else:
+                module.fail_json(msg='Cannot touch other than files and directories')
+
+            module.set_fs_attributes_if_different(file_args, True)
 
-        if prev_state not in ['file', 'directory', 'absent']:
-            module.fail_json(msg='Cannot touch other than files and directories')
-        if prev_state != 'absent':
-            try:
-                os.utime(path, None)
-            except OSError, e:
-                module.fail_json(path=path, msg='Error while touching existing target: %s' % str(e))
-        else:
-            try:
-                open(path, 'w').close()
-            except OSError, e:
-                module.fail_json(path=path, msg='Error, could not touch target: %s' % str(e))
-        module.set_file_attributes_if_different(file_args, True)
         module.exit_json(dest=path, changed=True)
 
-    else:
-        module.fail_json(path=path, msg='unexpected position reached')
+    module.fail_json(path=path, msg='unexpected position reached')
 
 # import module snippets
 from ansible.module_utils.basic import *

test/integration/roles/test_file/tasks/main.yml

@@ -164,5 +164,24 @@
     that:
       - "file11_result.uid == 1235"
 
+- name: fail to create soft link to non existant file
+  file: src=/noneexistant dest={{output_dir}}/soft2.txt state=link force=no
+  register: file12_result
+  ignore_errors: true
+
+- name: verify that link was not created
+  assert:
+    that:
+      - "file12_result.failed == true"
+
+- name: force creation soft link to non existant
+  file: src=/noneexistant dest={{output_dir}}/soft2.txt state=link force=yes
+  register: file13_result
+
+- name: verify that link was created
+  assert:
+    that:
+      - "file13_result.changed == true"
+
 - name: remote directory foobar
   file: path={{output_dir}}/foobar state=absent