From 9bd1f80d5c0ea564aa02460339ddbf5ec66312f0 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Tue, 5 Oct 2021 19:10:05 -0700 Subject: [PATCH] *: Always pass check= argument to subprocess.run() - Avoid flake8 warnings. - Makes the call more explicitly readable in case an exception is expected but check=True is not passed by mistake. Tests: - Many tests are skipped since the changes are considered trivial. check=False is already the default for subprocess.run() method. - actions/package: Install an app when it is not installed. - actions/upgrade: Run manual upgrades. - actions/users: Change a user password. Login. Create/remove a user. - actions/zoph: Restore a database. - container: On a fresh repository, run ./container up,ssh,stop,destroy for a testing container. - plinth/action_utils.py: Enable/disable an app that has a running service. Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- actions/monkeysphere | 3 +- actions/packages | 2 +- actions/storage | 2 +- actions/upgrades | 6 +-- actions/users | 7 ++- actions/zoph | 2 +- container | 46 +++++++++++-------- plinth/action_utils.py | 11 +++-- plinth/modules/backups/forms.py | 5 +- .../modules/backups/tests/test_ssh_remotes.py | 2 +- plinth/modules/email_server/audit/domain.py | 6 +-- plinth/modules/email_server/audit/home.py | 2 +- plinth/modules/email_server/audit/spam.py | 2 +- plinth/modules/email_server/lock.py | 2 +- plinth/modules/storage/tests/test_storage.py | 11 ++--- plinth/modules/upgrades/views.py | 3 +- plinth/modules/users/tests/test_actions.py | 5 +- 17 files changed, 62 insertions(+), 55 deletions(-) diff --git a/actions/monkeysphere b/actions/monkeysphere index c01e9246e..e9c892a7e 100755 --- a/actions/monkeysphere +++ b/actions/monkeysphere @@ -226,7 +226,8 @@ def _get_ssh_key_file_for_import(original_key_file, service): shutil.copy2(original_key_file, key_file) # Convert OpenSSH format to PEM subprocess.run( - ['ssh-keygen', '-p', '-N', '', '-m', 'PEM', '-f', key_file]) + ['ssh-keygen', '-p', '-N', '', '-m', 'PEM', '-f', key_file], + check=True) yield key_file diff --git a/actions/packages b/actions/packages index 1a907d86c..24517a3cd 100755 --- a/actions/packages +++ b/actions/packages @@ -96,7 +96,7 @@ def subcommand_install(arguments): if arguments.force_missing_configuration: extra_arguments += ['-o', 'Dpkg::Options::=--force-confmiss'] - subprocess.run(['dpkg', '--configure', '-a']) + subprocess.run(['dpkg', '--configure', '-a'], check=False) with apt_hold_freedombox(): run_apt_command(['--fix-broken', 'install']) returncode = run_apt_command(['install'] + extra_arguments + diff --git a/actions/storage b/actions/storage index 9ed372117..814c0adaf 100755 --- a/actions/storage +++ b/actions/storage @@ -273,7 +273,7 @@ def subcommand_mount(arguments): process = subprocess.run([ 'udisksctl', 'mount', '--block-device', arguments.block_device, '--no-user-interaction' - ]) + ], check=False) sys.exit(process.returncode) diff --git a/actions/upgrades b/actions/upgrades index 4edcb1be4..6abcaad9d 100755 --- a/actions/upgrades +++ b/actions/upgrades @@ -140,7 +140,7 @@ def parse_arguments(): def _run(): """Run unattended-upgrades""" - subprocess.run(['dpkg', '--configure', '-a']) + subprocess.run(['dpkg', '--configure', '-a'], check=False) run_apt_command(['--fix-broken', 'install']) # In case freedombox package was left in held state by an @@ -453,7 +453,7 @@ def _perform_dist_upgrade(): ['grub-pc grub-pc/install_devices_empty boolean true']) print('Running unattended-upgrade...', flush=True) - subprocess.run(['unattended-upgrade', '--verbose']) + subprocess.run(['unattended-upgrade', '--verbose'], check=False) # Remove obsolete packages that may prevent other packages from # upgrading. @@ -492,7 +492,7 @@ def _perform_dist_upgrade(): # Run unattended-upgrade once more to handle upgrading the # freedombox package. print('Running unattended-upgrade...', flush=True) - subprocess.run(['unattended-upgrade', '--verbose']) + subprocess.run(['unattended-upgrade', '--verbose'], check=False) # Restore original snapshots configuration. if snapshots_supported and apt_snapshots_enabled: diff --git a/actions/users b/actions/users index 5e0656cb8..d7f06db71 100755 --- a/actions/users +++ b/actions/users @@ -373,7 +373,7 @@ def set_samba_user(username, password): """ proc = subprocess.run(['smbpasswd', '-a', '-s', username], input='{0}\n{0}\n'.format(password).encode(), - stderr=subprocess.PIPE) + stderr=subprocess.PIPE, check=False) if proc.returncode != 0: raise RuntimeError('Unable to add Samba user: ', proc.stderr) @@ -580,13 +580,12 @@ def flush_cache(): action_utils.service_reload('apache2') -def _run(arguments, **kwargs): +def _run(arguments, check=True, **kwargs): """Run a command. Check return code and suppress output by default.""" env = dict(os.environ, LDAPSCRIPTS_CONF=LDAPSCRIPTS_CONF) kwargs['stdout'] = kwargs.get('stdout', subprocess.DEVNULL) kwargs['stderr'] = kwargs.get('stderr', subprocess.DEVNULL) - kwargs['check'] = kwargs.get('check', True) - return subprocess.run(arguments, env=env, **kwargs) + return subprocess.run(arguments, env=env, check=check, **kwargs) def main(): diff --git a/actions/zoph b/actions/zoph index 3b6b8a831..b32a5298c 100755 --- a/actions/zoph +++ b/actions/zoph @@ -128,7 +128,7 @@ def subcommand_dump_database(_): def subcommand_restore_database(_): """Restore database from file.""" db_name = _get_db_name() - subprocess.run(['mysqladmin', '--force', 'drop', db_name]) + subprocess.run(['mysqladmin', '--force', 'drop', db_name], check=False) subprocess.run(['mysqladmin', 'create', db_name], check=True) with open(DB_BACKUP_FILE, 'r') as db_restore_file: subprocess.run(['mysql', db_name], stdin=db_restore_file, check=True) diff --git a/container b/container index 8c8f56766..48194ac26 100755 --- a/container +++ b/container @@ -327,7 +327,7 @@ def _check_command(command): else: which = ['sudo', 'which', command] - process = subprocess.run(which, stdout=subprocess.DEVNULL) + process = subprocess.run(which, stdout=subprocess.DEVNULL, check=False) return process.returncode == 0 @@ -341,7 +341,8 @@ def _verify_dependencies(): # it leading to machinectl start failing first time after boot. See # https://github.com/systemd/systemd/issues/13130 . Workaround for old # versions of machinectl. Ignore errors. - subprocess.run(['sudo', 'modprobe', '--all', '--quiet', 'loop']) + subprocess.run(['sudo', 'modprobe', '--all', '--quiet', 'loop'], + check=False) dependencies = { 'systemd-nspawn': 'systemd-container', @@ -370,13 +371,13 @@ def _verify_dependencies(): ' '.join(missing_commands)) process = subprocess.run(['lsb_release', '--id', '--short'], - stdout=subprocess.PIPE) + stdout=subprocess.PIPE, check=False) if process.stdout.decode().strip() != 'Debian': sys.exit(1) logger.info('Running apt for missing packages: %s', ' '.join(missing_commands)) - subprocess.run(['sudo', 'apt', 'install'] + missing_packages) + subprocess.run(['sudo', 'apt', 'install'] + missing_packages, check=False) def _get_systemd_nspawn_version(): @@ -583,7 +584,7 @@ def _setup_nm_connection(distribution): connection_name = f'fbx-{distribution}-shared' process = subprocess.run( ['sudo', 'nmcli', 'connection', 'show', connection_name], - stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False) if not process.returncode: return @@ -600,7 +601,8 @@ def _setup_nm_connection(distribution): subprocess.run(['sudo', 'nmcli', 'connection', 'add'] + list(itertools.chain(*properties.items())), check=True) subprocess.run(['sudo', 'nmcli', 'connection', 'up', connection_name], - stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + check=False) def _setup_users(image_file): @@ -649,7 +651,7 @@ def _setup_ssh(image_file): logger.info('Generating SSH client key %s', key_file) subprocess.run( ['ssh-keygen', '-t', 'ed25519', '-N', '', '-f', - str(key_file)], stdout=subprocess.DEVNULL) + str(key_file)], stdout=subprocess.DEVNULL, check=True) public_key_file = key_file.with_suffix('.pub') public_key = public_key_file.read_bytes() @@ -713,33 +715,35 @@ VirtualEthernet=yes ''' nspawn_file = f'/run/systemd/nspawn/{machine_name}.nspawn' logger.info('Creating systemd-nspawn configuration: %s', nspawn_file) - subprocess.run(['sudo', 'rm', '--force', nspawn_file]) + subprocess.run(['sudo', 'rm', '--force', nspawn_file], check=False) subprocess.run(['sudo', 'tee', nspawn_file], input=nspawn_options.encode(), stdout=subprocess.DEVNULL, check=True) image_link = pathlib.Path(f'/var/lib/machines/{machine_name}.raw') logger.info('Linking systemd-nspawn image %s -> %s', image_link, image_file) - result = subprocess.run(['sudo', 'test', '-e', str(image_link)]) + result = subprocess.run( + ['sudo', 'test', '-e', str(image_link)], check=False) if not result.returncode: - result = subprocess.run(['sudo', 'test', '-L', str(image_link)]) + result = subprocess.run( + ['sudo', 'test', '-L', str(image_link)], check=False) if result.returncode: raise Exception(f'Image file {image_link} is not a symlink.') - subprocess.run(['sudo', 'rm', '--force', str(image_link)]) + subprocess.run(['sudo', 'rm', '--force', str(image_link)], check=False) subprocess.run([ 'sudo', 'ln', '--symbolic', str(image_file.resolve()), str(image_link) - ]) + ], check=False) def _get_machine_status(machine_name): """Return the running status of a container.""" process = subprocess.run(['sudo', 'machinectl', 'status', machine_name], stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL) + stderr=subprocess.DEVNULL, check=False) return process.returncode == 0 @@ -756,14 +760,14 @@ def _launch(image_file, distribution): f'fbx-{distribution}-shared') subprocess.run( ['sudo', 'nmcli', 'connection', 'up', f'fbx-{distribution}-shared'], - stdout=subprocess.DEVNULL) + stdout=subprocess.DEVNULL, check=False) def _stop(distribution): """Stop the container.""" machine_name = f'fbx-{distribution}' logger.info('Running `machinectl stop %s`', machine_name) - subprocess.run(['sudo', 'machinectl', 'stop', machine_name]) + subprocess.run(['sudo', 'machinectl', 'stop', machine_name], check=False) _wait_for(lambda: not _get_machine_status(machine_name)) @@ -771,7 +775,8 @@ def _terminate(distribution): """Terminal the container.""" machine_name = f'fbx-{distribution}' logger.info('Running `machinectl terminate %s`', machine_name) - subprocess.run(['sudo', 'machinectl', 'terminate', machine_name]) + subprocess.run(['sudo', 'machinectl', 'terminate', machine_name], + check=False) _wait_for(lambda: not _get_machine_status(machine_name)) @@ -781,16 +786,17 @@ def _destroy(distribution): image_link = pathlib.Path(f'/var/lib/machines/{machine_name}.raw') logger.info('Removing link to systemd-nspawn image %s', image_link) - subprocess.run(['sudo', 'rm', '--force', str(image_link)]) + subprocess.run(['sudo', 'rm', '--force', str(image_link)], check=False) nspawn_file = f'/run/systemd/nspawn/{machine_name}.nspawn' logger.info('Removing systemd-nspawn configuration: %s', nspawn_file) - subprocess.run(['sudo', 'rm', '--force', nspawn_file]) + subprocess.run(['sudo', 'rm', '--force', nspawn_file], check=False) overlay_folder = _get_overlay_folder(distribution) logger.info('Removing overlay folder with container written data: %s', overlay_folder) - subprocess.run(['sudo', 'rm', '-r', '--force', overlay_folder]) + subprocess.run(['sudo', 'rm', '-r', '--force', overlay_folder], + check=False) compressed_image = _get_compressed_image_path(distribution) image_file = compressed_image.with_suffix('') @@ -814,7 +820,7 @@ def _destroy(distribution): logger.info('Removing Network Manager connection %s', connection_name) result = subprocess.run( ['sudo', 'nmcli', 'connection', 'delete', connection_name], - capture_output=True) + capture_output=True, check=False) if result.returncode not in (0, 10): # nmcli failed and not due to 'Connection, device, or access point does # not exist.' See diff --git a/plinth/action_utils.py b/plinth/action_utils.py index baba9194b..ccc71a0e6 100644 --- a/plinth/action_utils.py +++ b/plinth/action_utils.py @@ -128,10 +128,10 @@ def service_action(service_name, action): """Perform the given action on the service_name.""" if is_systemd_running(): subprocess.run(['systemctl', action, service_name], - stdout=subprocess.DEVNULL) + stdout=subprocess.DEVNULL, check=False) else: subprocess.run(['service', service_name, action], - stdout=subprocess.DEVNULL) + stdout=subprocess.DEVNULL, check=False) def webserver_is_enabled(name, kind='config'): @@ -371,7 +371,7 @@ Owners: {package} env['DEBCONF_DB_OVERRIDE'] = 'File{' + override_file.name + \ ' readonly:true}' env['DEBIAN_FRONTEND'] = 'noninteractive' - subprocess.run(['dpkg-reconfigure', package], env=env) + subprocess.run(['dpkg-reconfigure', package], env=env, check=False) try: os.remove(override_file.name) @@ -419,7 +419,7 @@ def run_apt_command(arguments): env['DEBIAN_FRONTEND'] = 'noninteractive' process = subprocess.run(command, stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, close_fds=False, - env=env) + env=env, check=False) return process.returncode @@ -458,7 +458,8 @@ def apt_hold_freedombox(): def apt_unhold_freedombox(): """Remove any hold on freedombox package, and clear flag.""" subprocess.run(['apt-mark', 'unhold', 'freedombox'], - stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, + check=False) if apt_hold_flag.exists(): apt_hold_flag.unlink() diff --git a/plinth/modules/backups/forms.py b/plinth/modules/backups/forms.py index 627fa933c..ac70ad2fa 100644 --- a/plinth/modules/backups/forms.py +++ b/plinth/modules/backups/forms.py @@ -285,12 +285,13 @@ class VerifySshHostkeyForm(forms.Form): # Fetch public keys of ssh remote keyscan = subprocess.run(['ssh-keyscan', hostname], stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + stderr=subprocess.PIPE, check=False) keys = keyscan.stdout.decode().splitlines() error_message = keyscan.stderr.decode() if keyscan.returncode else None # Generate user-friendly fingerprints of public keys keygen = subprocess.run(['ssh-keygen', '-l', '-f', '-'], - input=keyscan.stdout, stdout=subprocess.PIPE) + input=keyscan.stdout, stdout=subprocess.PIPE, + check=False) fingerprints = keygen.stdout.decode().splitlines() return zip(keys, fingerprints), error_message diff --git a/plinth/modules/backups/tests/test_ssh_remotes.py b/plinth/modules/backups/tests/test_ssh_remotes.py index 5bc6688a8..d0b789ad2 100644 --- a/plinth/modules/backups/tests/test_ssh_remotes.py +++ b/plinth/modules/backups/tests/test_ssh_remotes.py @@ -38,7 +38,7 @@ def fixture_password(): def get_hashed_password(password): res = subprocess.run(['mkpasswd', '--method=md5', password], - stdout=subprocess.PIPE) + stdout=subprocess.PIPE, check=True) return res.stdout.decode().strip() diff --git a/plinth/modules/email_server/audit/domain.py b/plinth/modules/email_server/audit/domain.py index b9205292e..da8c77ea6 100644 --- a/plinth/modules/email_server/audit/domain.py +++ b/plinth/modules/email_server/audit/domain.py @@ -244,8 +244,8 @@ def _action_set_keys(): clean_dict[key] = clean_function(value) # Apply changes (postconf) - postconf_dict = dict(filter(lambda kv: not kv[0].startswith('_'), - clean_dict.items())) + postconf_dict = dict( + filter(lambda kv: not kv[0].startswith('_'), clean_dict.items())) postconf.set_many(postconf_dict) # Apply changes (special) @@ -258,7 +258,7 @@ def _action_set_keys(): with postconf.mutex.lock_all(): # systemctl reload postfix args = ['systemctl', 'reload', 'postfix'] - completed = subprocess.run(args, capture_output=True) + completed = subprocess.run(args, capture_output=True, check=False) if completed.returncode != 0: interproc.log_subprocess(completed) raise OSError('Could not reload postfix') diff --git a/plinth/modules/email_server/audit/home.py b/plinth/modules/email_server/audit/home.py index 2b5e24d41..3e7d54326 100644 --- a/plinth/modules/email_server/audit/home.py +++ b/plinth/modules/email_server/audit/home.py @@ -65,7 +65,7 @@ def action_mk(arg_type, user_info): args = ['sudo', '-n', '--user=#' + str(passwd.pw_uid)] args.extend(['/bin/sh', '-c', 'mkdir -p ~']) - completed = subprocess.run(args, capture_output=True) + completed = subprocess.run(args, capture_output=True, check=False) if completed.returncode != 0: interproc.log_subprocess(completed) raise OSError('Could not create home directory') diff --git a/plinth/modules/email_server/audit/spam.py b/plinth/modules/email_server/audit/spam.py index fb19f8dee..7809abd82 100644 --- a/plinth/modules/email_server/audit/spam.py +++ b/plinth/modules/email_server/audit/spam.py @@ -140,7 +140,7 @@ def _compile_sieve(): def _run_sievec(sieve_file): logger.info('Compiling sieve script %s', sieve_file) args = ['sievec', '--', sieve_file] - completed = subprocess.run(args, capture_output=True) + completed = subprocess.run(args, capture_output=True, check=False) if completed.returncode != 0: interproc.log_subprocess(completed) raise OSError('Sieve compilation failed: ' + sieve_file) diff --git a/plinth/modules/email_server/lock.py b/plinth/modules/email_server/lock.py index e3909039c..8619f1e6a 100644 --- a/plinth/modules/email_server/lock.py +++ b/plinth/modules/email_server/lock.py @@ -89,7 +89,7 @@ class Mutex: args.extend(['/bin/sh', '-c']) args.append('umask 177 && > ' + self.lock_path) - completed = subprocess.run(args, capture_output=True) + completed = subprocess.run(args, capture_output=True, check=False) if completed.returncode != 0: interproc.log_subprocess(completed) raise OSError('Could not create ' + self.lock_path) diff --git a/plinth/modules/storage/tests/test_storage.py b/plinth/modules/storage/tests/test_storage.py index db11baca0..e84ccdb1e 100644 --- a/plinth/modules/storage/tests/test_storage.py +++ b/plinth/modules/storage/tests/test_storage.py @@ -56,7 +56,7 @@ class Disk(): command = 'losetup --show --find {file}'.format( file=self.disk_file.name) process = subprocess.run(command.split(), stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + stderr=subprocess.PIPE, check=False) if process.returncode: if b'cannot find an unused loop device' in process.stderr: pytest.skip('Loopback devices not available') @@ -107,7 +107,7 @@ class Disk(): def _cleanup_loopback(self): """Undo the loopback device setup.""" - subprocess.run(['losetup', '--detach', self.device]) + subprocess.run(['losetup', '--detach', self.device], check=False) def _remove_disk_file(self): """Delete the disk_file.""" @@ -231,16 +231,15 @@ class TestActions: self.assert_aligned(partition_number) @staticmethod - def call_action(action_command, **kwargs): + def call_action(action_command, check=True, **kwargs): """Call the action script.""" test_directory = pathlib.Path(__file__).parent top_directory = (test_directory / '..' / '..' / '..' / '..').resolve() action_command[0] = top_directory / 'actions' / action_command[0] kwargs['stdout'] = kwargs.get('stdout', subprocess.DEVNULL) kwargs['stderr'] = kwargs.get('stderr', subprocess.DEVNULL) - kwargs['check'] = kwargs.get('check', True) env = dict(os.environ, PYTHONPATH=str(top_directory)) - return subprocess.run(action_command, env=env, **kwargs) + return subprocess.run(action_command, env=env, check=check, **kwargs) def check_action(self, action_command): """Return success/failure result of the action command.""" @@ -255,7 +254,7 @@ class TestActions: subprocess.run([ 'parted', '--script', self.device, 'align-check', 'opti', str(partition_number) - ]) + ], check=True) def assert_btrfs_file_system_healthy(self, partition_number): """Perform a successful ext4 file system check.""" diff --git a/plinth/modules/upgrades/views.py b/plinth/modules/upgrades/views.py index 3d17ca3a7..911fd3dfa 100644 --- a/plinth/modules/upgrades/views.py +++ b/plinth/modules/upgrades/views.py @@ -114,7 +114,8 @@ def get_log(): def _is_updating(): """Check if manually triggered update is running.""" command = ['systemctl', 'is-active', 'freedombox-manual-upgrade'] - result = subprocess.run(command, capture_output=True, text=True) + result = subprocess.run(command, capture_output=True, text=True, + check=False) return str(result.stdout).startswith('activ') # 'active' or 'activating' diff --git a/plinth/modules/users/tests/test_actions.py b/plinth/modules/users/tests/test_actions.py index 5c038e814..832077d4e 100644 --- a/plinth/modules/users/tests/test_actions.py +++ b/plinth/modules/users/tests/test_actions.py @@ -138,12 +138,11 @@ def fixture_auto_cleanup_users_groups(needs_root, load_cfg): _delete_group(group) -def _call_action(arguments, **kwargs): +def _call_action(arguments, check=True, **kwargs): """Call the action script.""" kwargs['stdout'] = kwargs.get('stdout', subprocess.PIPE) kwargs['stderr'] = kwargs.get('stderr', subprocess.PIPE) - kwargs['check'] = kwargs.get('check', True) - return subprocess.run([_action_file()] + arguments, **kwargs) + return subprocess.run([_action_file()] + arguments, check=check, **kwargs) def _create_user(username=None, groups=None):