From bd28c244795719851c5b186825997a26f2fbc713 Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Fri, 11 May 2018 05:16:07 +0200 Subject: [PATCH 01/12] [mod] blank lines to make code more readable --- src/yunohost/service.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index ef61154bd..f9bc14a03 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -323,12 +323,15 @@ def service_regen_conf(names=[], with_diff=False, force=False, dry_run=False, # create the pending conf directory for the service service_pending_path = os.path.join(PENDING_CONF_DIR, name) filesystem.mkdir(service_pending_path, 0755, True, uid='admin') + # return the arguments to pass to the script return pre_args + [service_pending_path, ] + pre_result = hook_callback('conf_regen', names, pre_callback=_pre_call) # Update the services name names = pre_result['succeed'].keys() + if not names: raise MoulinetteError(errno.EIO, m18n.n('service_regenconf_failed', @@ -386,6 +389,7 @@ def service_regen_conf(names=[], with_diff=False, force=False, dry_run=False, 'service_conf_file_manually_removed', conf=system_path)) conf_status = 'removed' + # -> system conf is not managed yet elif not saved_hash: logger.debug("> system conf is not managed yet") @@ -409,6 +413,7 @@ def service_regen_conf(names=[], with_diff=False, force=False, dry_run=False, logger.warning(m18n.n('service_conf_file_kept_back', conf=system_path, service=service)) conf_status = 'unmanaged' + # -> system conf has not been manually modified elif system_hash == saved_hash: if to_remove: @@ -421,6 +426,7 @@ def service_regen_conf(names=[], with_diff=False, force=False, dry_run=False, logger.debug("> system conf is already up-to-date") os.remove(pending_path) continue + else: logger.debug("> system conf has been manually modified") if system_hash == new_hash: @@ -457,6 +463,7 @@ def service_regen_conf(names=[], with_diff=False, force=False, dry_run=False, 'service_conf_updated' if not dry_run else 'service_conf_would_be_updated', service=service)) + if succeed_regen and not dry_run: _update_conf_hashes(service, conf_hashes) @@ -480,6 +487,7 @@ def service_regen_conf(names=[], with_diff=False, force=False, dry_run=False, else: regen_conf_files = '' return post_args + [regen_conf_files, ] + hook_callback('conf_regen', names, pre_callback=_pre_call) return result @@ -678,25 +686,33 @@ def _get_pending_conf(services=[]): """ result = {} + if not os.path.isdir(PENDING_CONF_DIR): return result + if not services: services = os.listdir(PENDING_CONF_DIR) + for name in services: service_pending_path = os.path.join(PENDING_CONF_DIR, name) + if not os.path.isdir(service_pending_path): continue + path_index = len(service_pending_path) service_conf = {} + for root, dirs, files in os.walk(service_pending_path): for filename in files: pending_path = os.path.join(root, filename) service_conf[pending_path[path_index:]] = pending_path + if service_conf: result[name] = service_conf else: # remove empty directory shutil.rmtree(service_pending_path, ignore_errors=True) + return result @@ -708,9 +724,11 @@ def _get_conf_hashes(service): if service not in services: logger.debug("Service %s is not in services.yml yet.", service) return {} + elif services[service] is None or 'conffiles' not in services[service]: logger.debug("No configuration files for service %s.", service) return {} + else: return services[service]['conffiles'] @@ -743,11 +761,14 @@ def _process_regen_conf(system_conf, new_conf=None, save=True): backup_path = os.path.join(BACKUP_CONF_DIR, '{0}-{1}'.format( system_conf.lstrip('/'), time.strftime("%Y%m%d.%H%M%S"))) backup_dir = os.path.dirname(backup_path) + if not os.path.isdir(backup_dir): filesystem.mkdir(backup_dir, 0755, True) + shutil.copy2(system_conf, backup_path) logger.info(m18n.n('service_conf_file_backed_up', conf=system_conf, backup=backup_path)) + try: if not new_conf: os.remove(system_conf) @@ -755,8 +776,10 @@ def _process_regen_conf(system_conf, new_conf=None, save=True): conf=system_conf)) else: system_dir = os.path.dirname(system_conf) + if not os.path.isdir(system_dir): filesystem.mkdir(system_dir, 0755, True) + shutil.copyfile(new_conf, system_conf) logger.info(m18n.n('service_conf_file_updated', conf=system_conf)) @@ -766,6 +789,7 @@ def _process_regen_conf(system_conf, new_conf=None, save=True): conf=system_conf), exc_info=1) return False + elif new_conf: try: copy_succeed = os.path.samefile(system_conf, new_conf) @@ -777,8 +801,10 @@ def _process_regen_conf(system_conf, new_conf=None, save=True): conf=system_conf, new=new_conf), exc_info=1) return False + return True + def manually_modified_files(): # We do this to have --quiet, i.e. don't throw a whole bunch of logs From 1fab47cbeab4e1a2ddea080090a76094992b6b52 Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Fri, 11 May 2018 22:19:54 +0200 Subject: [PATCH 02/12] [mod] reduce indentation level --- src/yunohost/service.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index f9bc14a03..527d7a2c7 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -292,14 +292,19 @@ def service_regen_conf(names=[], with_diff=False, force=False, dry_run=False, # Return the list of pending conf if list_pending: pending_conf = _get_pending_conf(names) - if with_diff: - for service, conf_files in pending_conf.items(): - for system_path, pending_path in conf_files.items(): - pending_conf[service][system_path] = { - 'pending_conf': pending_path, - 'diff': _get_files_diff( - system_path, pending_path, True), - } + + if not with_diff: + return pending_conf + + for service, conf_files in pending_conf.items(): + for system_path, pending_path in conf_files.items(): + + pending_conf[service][system_path] = { + 'pending_conf': pending_path, + 'diff': _get_files_diff( + system_path, pending_path, True), + } + return pending_conf # Clean pending conf directory From 7435cfdea391e6793735174ffe83a9b407610dfc Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Fri, 11 May 2018 22:20:07 +0200 Subject: [PATCH 03/12] [mod] simplify code, give more verbose debug output --- src/yunohost/service.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 527d7a2c7..f965343d4 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -511,11 +511,11 @@ def _run_service_command(action, service): if service not in services.keys(): raise MoulinetteError(errno.EINVAL, m18n.n('service_unknown', service=service)) - cmd = None - if action in ['start', 'stop', 'restart', 'reload', 'enable', 'disable']: - cmd = 'systemctl %s %s' % (action, service) - else: - raise ValueError("Unknown action '%s'" % action) + possible_actions = ['start', 'stop', 'restart', 'reload', 'enable', 'disable'] + if action not in possible_actions: + raise ValueError("Unknown action '%s', available actions are: %s" % (action, ", ".join(possible_actions))) + + cmd = 'systemctl %s %s' % (action, service) need_lock = services[service].get('need_lock', False) \ and action in ['start', 'stop', 'restart', 'reload'] From 1d59738085628b49952b3e8efc5cb3bf5b875f56 Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Fri, 11 May 2018 22:57:37 +0200 Subject: [PATCH 04/12] [fix] always remove lock if needed --- src/yunohost/service.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index f965343d4..91b060a57 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -530,14 +530,17 @@ def _run_service_command(action, service): PID = _give_lock(action, service, p) # Wait for the command to complete p.communicate() - # Remove the lock if one was given - if need_lock and PID != 0: - _remove_lock(PID) except subprocess.CalledProcessError as e: # TODO: Log output? logger.warning(m18n.n('service_cmd_exec_failed', command=' '.join(e.cmd))) return False + + finally: + # Remove the lock if one was given + if need_lock and PID != 0: + _remove_lock(PID) + return True From ebe5cab0999cf51e99ab66bbbcb2482795c55223 Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Fri, 11 May 2018 23:27:24 +0200 Subject: [PATCH 05/12] [mod] add warning comment about unconcurrency safe _remove_lock function --- src/yunohost/service.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 91b060a57..20819ae98 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -573,6 +573,7 @@ def _give_lock(action, service, p): return son_PID def _remove_lock(PID_to_remove): + # FIXME ironically not concurrency safe because it's not atomic... PIDs = filesystem.read_file(MOULINETTE_LOCK).split("\n") PIDs_to_keep = [ PID for PID in PIDs if int(PID) != PID_to_remove ] From 14c270cebdc67a1123331237cee6dba347ee7623 Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Sat, 12 May 2018 00:26:22 +0200 Subject: [PATCH 06/12] [mod] offset is never used --- src/yunohost/service.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 20819ae98..9b4b8348e 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -613,7 +613,7 @@ def _save_services(services): yaml.safe_dump(services, f, default_flow_style=False) -def _tail(file, n, offset=None): +def _tail(file, n): """ Reads a n lines from f with an offset of offset lines. The return value is a tuple in the form ``(lines, has_more)`` where `has_more` is @@ -621,7 +621,7 @@ def _tail(file, n, offset=None): """ avg_line_length = 74 - to_read = n + (offset or 0) + to_read = n try: with open(file, 'r') as f: @@ -635,7 +635,7 @@ def _tail(file, n, offset=None): pos = f.tell() lines = f.read().splitlines() if len(lines) >= to_read or pos == 0: - return lines[-to_read:offset and -offset or None] + return lines[-to_read] avg_line_length *= 1.3 except IOError: From ac6a14055dc48faa67b147b07ab6ab79cedee03b Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Sat, 12 May 2018 02:22:26 +0200 Subject: [PATCH 07/12] [mod] lisibility --- src/yunohost/service.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 9b4b8348e..b5908741f 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -632,10 +632,13 @@ def _tail(file, n): # woops. apparently file is smaller than what we want # to step back, go to the beginning instead f.seek(0) + pos = f.tell() lines = f.read().splitlines() + if len(lines) >= to_read or pos == 0: return lines[-to_read] + avg_line_length *= 1.3 except IOError: From b7e946517b998fa58ce0cf839421ace8da19adb1 Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Sat, 12 May 2018 02:22:38 +0200 Subject: [PATCH 08/12] [mod] more debug output --- src/yunohost/service.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index b5908741f..e37cd0062 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -641,7 +641,8 @@ def _tail(file, n): avg_line_length *= 1.3 - except IOError: + except IOError as e: + logger.warning("Error while tailing file '%s': %s", file, e, exc_info=1) return [] From 3155def9dd13e3ccbc3e76862e754aea563dbf8d Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Sat, 12 May 2018 02:55:08 +0200 Subject: [PATCH 09/12] [mod] simplify code --- src/yunohost/service.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index e37cd0062..0f5c1beb4 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -654,25 +654,25 @@ def _get_files_diff(orig_file, new_file, as_string=False, skip_header=True): header can also be removed if skip_header is True. """ - contents = [[], []] - for i, path in enumerate((orig_file, new_file)): - try: - with open(path, 'r') as f: - contents[i] = f.readlines() - except IOError: - pass + with open(orig_file, 'r') as orig_file: + orig_file = orig_file.readlines() + + with open(new_file, 'r') as new_file: + new_file.readlines() # Compare files and format output - diff = unified_diff(contents[0], contents[1]) + diff = unified_diff(orig_file, new_file) + if skip_header: - for i in range(2): - try: - next(diff) - except: - break + try: + next(diff) + next(diff) + except: + pass + if as_string: - result = ''.join(line for line in diff) - return result.rstrip() + return ''.join(diff).rstrip() + return diff From d15ca90eedb656258f6b1cad69464123e66b5d3b Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Sat, 12 May 2018 02:59:09 +0200 Subject: [PATCH 10/12] [mod] add more debug output --- src/yunohost/service.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 0f5c1beb4..ee9ff33f1 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -679,11 +679,14 @@ def _get_files_diff(orig_file, new_file, as_string=False, skip_header=True): def _calculate_hash(path): """Calculate the MD5 hash of a file""" hasher = hashlib.md5() + try: with open(path, 'rb') as f: hasher.update(f.read()) return hasher.hexdigest() - except IOError: + + except IOError as e: + logger.warning("Error while calculating file '%s' hash: %s", path, e, exc_info=1) return None From 249994785f2837114a688feca492dd93173c7cbb Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Sat, 12 May 2018 03:12:52 +0200 Subject: [PATCH 11/12] [mod] more debug output --- src/yunohost/service.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index ee9ff33f1..99726b720 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -799,7 +799,8 @@ def _process_regen_conf(system_conf, new_conf=None, save=True): shutil.copyfile(new_conf, system_conf) logger.info(m18n.n('service_conf_file_updated', conf=system_conf)) - except: + except Exception as e: + logger.warning("Exception while trying to regenerate conf '%s': %s", system_conf, e, exc_info=1) if not new_conf and os.path.exists(system_conf): logger.warning(m18n.n('service_conf_file_remove_failed', conf=system_conf), From 5f2f262c57aaabe914781a8679e8ec1e3497cefb Mon Sep 17 00:00:00 2001 From: Laurent Peuch Date: Sat, 12 May 2018 03:13:03 +0200 Subject: [PATCH 12/12] [doc] add comment explaining situation --- src/yunohost/service.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 99726b720..27b7f378b 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -809,6 +809,9 @@ def _process_regen_conf(system_conf, new_conf=None, save=True): elif new_conf: try: + # From documentation: + # Raise an exception if an os.stat() call on either pathname fails. + # (os.stats returns a series of information from a file like type, size...) copy_succeed = os.path.samefile(system_conf, new_conf) except: copy_succeed = False