From 2d25577e1104101c67bb5ac41790693780a6d51b Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Thu, 13 Mar 2014 23:07:35 -0400 Subject: [PATCH] Fixes and cleanup to file functions and module - unified set attribute functions ... not sure why 2 identical functions exist with diff names, now there are 3 while i repoint all modules to 1 - fixed issue with symlinks being created w/o existing src when force=no - refactored conditionals, simplified where possible - added tests for symlink to nonexistant source, with both force options - made symlink on existing attomic (force) --- lib/ansible/module_utils/basic.py | 21 +- library/files/file | 187 ++++++++---------- .../roles/test_file/tasks/main.yml | 19 ++ 3 files changed, 112 insertions(+), 115 deletions(-) diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 42b9d3d669b..2f0c0f61aca 100644 --- a/lib/ansible/module_utils/basic.py +++ b/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) diff --git a/library/files/file b/library/files/file index 7a038c9f362..4d6fc0e7b40 100644 --- a/library/files/file +++ b/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 * diff --git a/test/integration/roles/test_file/tasks/main.yml b/test/integration/roles/test_file/tasks/main.yml index 174f66a9fba..588c1b6747b 100644 --- a/test/integration/roles/test_file/tasks/main.yml +++ b/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