[systemd-devel] [PATCHi v2] systemctl: add add-wants and add-requires verbs
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Wed Sep 24 09:15:08 PDT 2014
On Wed, Sep 24, 2014 at 04:27:29PM +0200, Lukas Nykryn wrote:
> ---
>
> Changes in v2
>
> - new selinux_unit_access_check_strv
> - only one dbus call AddInstallDependencyUnitFiles
> - small change in manpage
>
> TODO | 1 -
> man/systemctl.xml | 21 ++++++++
> src/core/dbus-manager.c | 84 ++++++++++++++++++-----------
> src/core/org.freedesktop.systemd1.conf | 4 ++
> src/core/selinux-access.c | 29 ++++++++++
> src/core/selinux-access.h | 3 ++
> src/shared/install.c | 94 ++++++++++++++++++++++++++++++---
> src/shared/install.h | 11 ++++
> src/systemctl/systemctl.c | 96 ++++++++++++++++++++++++++++++++++
> 9 files changed, 305 insertions(+), 38 deletions(-)
>
> diff --git a/TODO b/TODO
> index 9ac6fac..923cbb8 100644
> --- a/TODO
> +++ b/TODO
> @@ -450,7 +450,6 @@ Features:
> - "systemctl mask" should find all names by which a unit is accessible
> (i.e. by scanning for symlinks to it) and link them all to /dev/null
> - systemctl list-unit-files should list generated files (and probably with a new state "generated" for them, or so)
> - - systemctl: maybe add "systemctl add-wants" or so...
>
> * timer units:
> - timer units should get the ability to trigger when:
> diff --git a/man/systemctl.xml b/man/systemctl.xml
> index b28a3b7..c4ee308 100644
> --- a/man/systemctl.xml
> +++ b/man/systemctl.xml
> @@ -1098,6 +1098,27 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service
> </varlistentry>
>
> <varlistentry>
> + <term><command>add-wants|add-requires <replaceable>TARGET</replaceable>
> + <replaceable>NAME</replaceable>...</command>
> + </term>
> +
> + <listitem>
> + <para>Enables one or more units similarly to <command>enable</command>,
> + but instead of looking to <literal>[Install]</literal> section of
> + the unit file, adds <literal>Wants</literal>
> + resp. <literal>Requires</literal> dependency to the specified
> + <replaceable>TARGET</replaceable>.
> + </para>
> +
> + <para>This command honors <option>--system</option>,
> + <option>--user</option>, <option>--runtime</option> and
> + <option>--global</option> in a similar way as
> + <command>enable</command>.</para>
> +
> + </listitem>
> + </varlistentry>
> +
> + <varlistentry>
> <term><command>link <replaceable>FILENAME</replaceable>...</command></term>
>
> <listitem>
> diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c
> index 533ce43..921a614 100644
> --- a/src/core/dbus-manager.c
> +++ b/src/core/dbus-manager.c
> @@ -1562,9 +1562,6 @@ static int method_enable_unit_files_generic(
> sd_bus_error *error) {
>
> _cleanup_strv_free_ char **l = NULL;
> -#ifdef HAVE_SELINUX
> - char **i;
> -#endif
> UnitFileChange *changes = NULL;
> unsigned n_changes = 0;
> UnitFileScope scope;
> @@ -1588,18 +1585,9 @@ static int method_enable_unit_files_generic(
> if (r < 0)
> return r;
>
> -#ifdef HAVE_SELINUX
> - STRV_FOREACH(i, l) {
> - Unit *u;
> -
> - u = manager_get_unit(m, *i);
> - if (u) {
> - r = selinux_unit_access_check(u, message, verb, error);
> - if (r < 0)
> - return r;
> - }
> - }
> -#endif
> + r = selinux_unit_access_check_strv(l, message, m, verb, error);
> + if (r < 0)
> + return r;
>
> scope = m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER;
>
> @@ -1637,9 +1625,6 @@ static int method_mask_unit_files(sd_bus *bus, sd_bus_message *message, void *us
> static int method_preset_unit_files_with_mode(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
>
> _cleanup_strv_free_ char **l = NULL;
> -#ifdef HAVE_SELINUX
> - char **i;
> -#endif
> UnitFileChange *changes = NULL;
> unsigned n_changes = 0;
> Manager *m = userdata;
> @@ -1674,18 +1659,9 @@ static int method_preset_unit_files_with_mode(sd_bus *bus, sd_bus_message *messa
> return -EINVAL;
> }
>
> -#ifdef HAVE_SELINUX
> - STRV_FOREACH(i, l) {
> - Unit *u;
> -
> - u = manager_get_unit(m, *i);
> - if (u) {
> - r = selinux_unit_access_check(u, message, "enable", error);
> - if (r < 0)
> - return r;
> - }
> - }
> -#endif
> + r = selinux_unit_access_check_strv(l, message, m, "enable", error);
> + if (r < 0)
> + return r;
>
> scope = m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER;
>
> @@ -1828,6 +1804,53 @@ static int method_preset_all_unit_files(sd_bus *bus, sd_bus_message *message, vo
> return reply_unit_file_changes_and_free(m, bus, message, -1, changes, n_changes);
> }
I don't understand the name "method_add_install_dependency_unit_files".
Why not just "method_add_dependencies"? After all, this is not like install,
and does not work on the level of unit files, but units.
Similarly for the dbus method name "AddInstallDependencyUnitFiles".
> +static int method_add_install_dependency_unit_files(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
> + _cleanup_strv_free_ char **l = NULL;
> + Manager *m = userdata;
> + UnitFileChange *changes = NULL;
> + unsigned n_changes = 0;
> + UnitFileScope scope;
> + int runtime, force, r;
> + char *target;
> + char *type;
> + UnitFileInstallDependency dep;
> +
> + assert(bus);
> + assert(message);
> + assert(m);
> +
> + r = bus_verify_manage_unit_files_async(m, message, error);
> + if (r < 0)
> + return r;
> + if (r == 0)
> + return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */
> +
> + r = sd_bus_message_read_strv(message, &l);
> + if (r < 0)
> + return r;
> +
> + r = sd_bus_message_read(message, "ssbb", &target, &type, &runtime, &force);
> + if (r < 0)
> + return r;
> +
> + dep = unit_file_install_dependency_from_string(type);
> +
> + if (dep < 0)
> + return -EINVAL;
> +
> + r = selinux_unit_access_check_strv(l, message, m, "enable", error);
> + if (r < 0)
> + return r;
> +
> + scope = m->running_as == SYSTEMD_SYSTEM ? UNIT_FILE_SYSTEM : UNIT_FILE_USER;
> +
> + r = unit_file_add_install_dependency(scope, runtime, NULL, l, target, dep, force, &changes, &n_changes);
> + if (r < 0)
> + return r;
> +
> + return reply_unit_file_changes_and_free(m, bus, message, -1, changes, n_changes);
> +}
> +
> const sd_bus_vtable bus_manager_vtable[] = {
> SD_BUS_VTABLE_START(0),
>
> @@ -1918,6 +1941,7 @@ const sd_bus_vtable bus_manager_vtable[] = {
> SD_BUS_METHOD("SetDefaultTarget", "sb", "a(sss)", method_set_default_target, SD_BUS_VTABLE_UNPRIVILEGED),
> SD_BUS_METHOD("GetDefaultTarget", NULL, "s", method_get_default_target, SD_BUS_VTABLE_UNPRIVILEGED),
> SD_BUS_METHOD("PresetAllUnitFiles", "sbb", "a(sss)", method_preset_all_unit_files, SD_BUS_VTABLE_UNPRIVILEGED),
> + SD_BUS_METHOD("AddInstallDependencyUnitFiles", "asssbb", "a(sss)", method_add_install_dependency_unit_files, SD_BUS_VTABLE_UNPRIVILEGED),
>
> SD_BUS_SIGNAL("UnitNew", "so", 0),
> SD_BUS_SIGNAL("UnitRemoved", "so", 0),
> diff --git a/src/core/org.freedesktop.systemd1.conf b/src/core/org.freedesktop.systemd1.conf
> index 3e13825..f9a83d8 100644
> --- a/src/core/org.freedesktop.systemd1.conf
> +++ b/src/core/org.freedesktop.systemd1.conf
> @@ -199,6 +199,10 @@
> send_member="PresetAllUnitFiles"/>
>
> <allow send_destination="org.freedesktop.systemd1"
> + send_interface="org.freedesktop.systemd1.Manager"
> + send_member="AddInstallDependencyUnitFiles"/>
> +
> + <allow send_destination="org.freedesktop.systemd1"
> send_interface="org.freedesktop.systemd1.Job"
> send_member="Cancel"/>
>
> diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c
> index cdbfb83..184f202 100644
> --- a/src/core/selinux-access.c
> +++ b/src/core/selinux-access.c
> @@ -250,6 +250,27 @@ finish:
> return r;
> }
>
> +int selinux_unit_access_check_strv(char **units,
> + sd_bus_message *message,
> + Manager *m,
> + const char *permission,
> + sd_bus_error *error) {
> + char **i;
> + Unit *u;
> + int r;
> +
> + STRV_FOREACH(i, units) {
> + u = manager_get_unit(m, *i);
> + if (u) {
> + r = selinux_unit_access_check(u, message, permission, error);
> + if (r < 0)
> + return r;
> + }
> + }
> +
> + return 0;
> +}
> +
> #else
>
> int selinux_generic_access_check(
> @@ -264,4 +285,12 @@ int selinux_generic_access_check(
> void selinux_access_free(void) {
> }
>
> +int selinux_unit_access_check_strv(char **units,
> + sd_bus_message *message,
> + Manager *m,
> + const char *permission,
> + sd_bus_error *error) {
> + return 0;
> +}
> +
> #endif
> diff --git a/src/core/selinux-access.h b/src/core/selinux-access.h
> index 27d9e14..6a4362a 100644
> --- a/src/core/selinux-access.h
> +++ b/src/core/selinux-access.h
> @@ -24,11 +24,14 @@
> #include "sd-bus.h"
> #include "bus-error.h"
> #include "bus-util.h"
> +#include "manager.h"
>
> void selinux_access_free(void);
>
> int selinux_generic_access_check(sd_bus_message *message, const char *path, const char *permission, sd_bus_error *error);
>
> +int selinux_unit_access_check_strv(char **units, sd_bus_message *message, Manager *m, const char *permission, sd_bus_error *error);
> +
> #ifdef HAVE_SELINUX
>
> #define selinux_access_check(message, permission, error) \
> diff --git a/src/shared/install.c b/src/shared/install.c
> index 61e572b..da9a0e9 100644
> --- a/src/shared/install.c
> +++ b/src/shared/install.c
> @@ -1091,7 +1091,8 @@ static int unit_file_search(
> InstallInfo *info,
> LookupPaths *paths,
> const char *root_dir,
> - bool allow_symlink) {
> + bool allow_symlink,
> + bool load) {
>
> char **p;
> int r;
> @@ -1112,7 +1113,11 @@ static int unit_file_search(
> if (!path)
> return -ENOMEM;
>
> - r = unit_file_load(c, info, path, root_dir, allow_symlink);
> + if (load)
> + r = unit_file_load(c, info, path, root_dir, allow_symlink);
> + else
> + r = access(path, F_OK) ? -errno : 0;
> +
> if (r >= 0) {
> info->path = path;
> path = NULL;
> @@ -1141,7 +1146,11 @@ static int unit_file_search(
> if (!path)
> return -ENOMEM;
>
> - r = unit_file_load(c, info, path, root_dir, allow_symlink);
> + if (load)
> + r = unit_file_load(c, info, path, root_dir, allow_symlink);
> + else
> + r = access(path, F_OK) ? -errno : 0;
Wouldn't it be nicer to push the 'load' parameter into
unit_file_load(), and do the check there? I think that with
your patch the support for root_dir is missing.
> +
> if (r >= 0) {
> info->path = path;
> path = NULL;
> @@ -1174,7 +1183,7 @@ static int unit_file_can_install(
>
> assert_se(i = hashmap_first(c.will_install));
>
> - r = unit_file_search(&c, i, paths, root_dir, allow_symlink);
> + r = unit_file_search(&c, i, paths, root_dir, allow_symlink, true);
>
> if (r >= 0)
> r =
> @@ -1401,7 +1410,7 @@ static int install_context_apply(
>
> assert_se(hashmap_move_one(c->have_installed, c->will_install, i->name) == 0);
>
> - q = unit_file_search(c, i, paths, root_dir, false);
> + q = unit_file_search(c, i, paths, root_dir, false, true);
> if (q < 0) {
> if (r >= 0)
> r = q;
> @@ -1442,7 +1451,7 @@ static int install_context_mark_for_removal(
>
> assert_se(hashmap_move_one(c->have_installed, c->will_install, i->name) == 0);
>
> - q = unit_file_search(c, i, paths, root_dir, false);
> + q = unit_file_search(c, i, paths, root_dir, false, true);
> if (q == -ENOENT) {
> /* do nothing */
> } else if (q < 0) {
> @@ -1488,6 +1497,70 @@ static int install_context_mark_for_removal(
> return r;
> }
>
> +int unit_file_add_install_dependency(
> + UnitFileScope scope,
> + bool runtime,
> + const char *root_dir,
> + char **files,
> + char *target,
> + UnitFileInstallDependency dep,
> + bool force,
> + UnitFileChange **changes,
> + unsigned *n_changes) {
> +
> + _cleanup_lookup_paths_free_ LookupPaths paths = {};
> + _cleanup_(install_context_done) InstallContext c = {};
> + _cleanup_free_ char *config_path = NULL;
> + char **i;
> + int r;
> + InstallInfo *info;
> +
> + assert(scope >= 0);
> + assert(scope < _UNIT_FILE_SCOPE_MAX);
> +
> + r = lookup_paths_init_from_scope(&paths, scope, root_dir);
> + if (r < 0)
> + return r;
> +
> + r = get_config_path(scope, runtime, root_dir, &config_path);
> + if (r < 0)
> + return r;
> +
> + STRV_FOREACH(i, files) {
> + r = install_info_add_auto(&c, *i);
> + if (r < 0)
> + return r;
> + }
> +
> + while ((info = hashmap_first(c.will_install))) {
> + r = hashmap_ensure_allocated(&c.have_installed, &string_hash_ops);
> + if (r < 0)
> + return r;
> +
> + assert_se(hashmap_move_one(c.have_installed, c.will_install, info->name) == 0);
> +
> + r = unit_file_search(&c, info, &paths, root_dir, false, false);
> + if (r < 0)
> + return r;
> +
> + if (dep == UNIT_FILE_INSTALL_DEPENDENCY_WANTS)
> + r = strv_extend(&info->wanted_by, target);
> + else if (dep == UNIT_FILE_INSTALL_DEPENDENCY_REQUIRES)
> + r = strv_extend(&info->required_by, target);
> + else
> + r = -EINVAL;
> +
> + if (r < 0)
> + return r;
> +
> + r = install_info_apply(info, &paths, config_path, root_dir, force, changes, n_changes);
> + if (r < 0)
> + return r;
> + }
> +
> + return 0;
> +}
> +
> int unit_file_enable(
> UnitFileScope scope,
> bool runtime,
> @@ -1624,7 +1697,7 @@ int unit_file_set_default(
>
> assert_se(i = hashmap_first(c.will_install));
>
> - r = unit_file_search(&c, i, &paths, root_dir, false);
> + r = unit_file_search(&c, i, &paths, root_dir, false, true);
> if (r < 0)
> return r;
>
> @@ -2179,3 +2252,10 @@ static const char* const unit_file_preset_mode_table[_UNIT_FILE_PRESET_MAX] = {
> };
>
> DEFINE_STRING_TABLE_LOOKUP(unit_file_preset_mode, UnitFilePresetMode);
> +
> +static const char* const unit_file_install_dependency_table[_UNIT_FILE_INSTALL_DEPENDENCY_MAX] = {
> + [UNIT_FILE_INSTALL_DEPENDENCY_WANTS] = "wants",
> + [UNIT_FILE_INSTALL_DEPENDENCY_REQUIRES] = "requires",
> +};
> +
> +DEFINE_STRING_TABLE_LOOKUP(unit_file_install_dependency, UnitFileInstallDependency);
> diff --git a/src/shared/install.h b/src/shared/install.h
> index ff16d9f..e8a4c57 100644
> --- a/src/shared/install.h
> +++ b/src/shared/install.h
> @@ -53,6 +53,13 @@ typedef enum UnitFilePresetMode {
> _UNIT_FILE_PRESET_INVALID = -1
> } UnitFilePresetMode;
>
> +typedef enum UnitFileInstallDependency {
> + UNIT_FILE_INSTALL_DEPENDENCY_WANTS,
> + UNIT_FILE_INSTALL_DEPENDENCY_REQUIRES,
> + _UNIT_FILE_INSTALL_DEPENDENCY_MAX,
> + _UNIT_FILE_INSTALL_DEPENDENCY_INVALID = -1
> +} UnitFileInstallDependency;
> +
> typedef enum UnitFileChangeType {
> UNIT_FILE_SYMLINK,
> UNIT_FILE_UNLINK,
> @@ -93,6 +100,7 @@ int unit_file_mask(UnitFileScope scope, bool runtime, const char *root_dir, char
> int unit_file_unmask(UnitFileScope scope, bool runtime, const char *root_dir, char **files, UnitFileChange **changes, unsigned *n_changes);
> int unit_file_set_default(UnitFileScope scope, const char *root_dir, const char *file, bool force, UnitFileChange **changes, unsigned *n_changes);
> int unit_file_get_default(UnitFileScope scope, const char *root_dir, char **name);
> +int unit_file_add_install_dependency(UnitFileScope scope, bool runtime, const char *root_dir, char **files, char *target, UnitFileInstallDependency dep, bool force, UnitFileChange **changes, unsigned *n_changes);
>
> UnitFileState unit_file_get_state(UnitFileScope scope, const char *root_dir, const char *filename);
>
> @@ -111,3 +119,6 @@ UnitFileChangeType unit_file_change_type_from_string(const char *s) _pure_;
>
> const char *unit_file_preset_mode_to_string(UnitFilePresetMode m) _const_;
> UnitFilePresetMode unit_file_preset_mode_from_string(const char *s) _pure_;
> +
> +const char *unit_file_install_dependency_to_string(UnitFileInstallDependency d) _const_;
> +UnitFileInstallDependency unit_file_install_dependency_from_string(const char *s) _pure_;
> diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
> index 9012128..6eaab87 100644
> --- a/src/systemctl/systemctl.c
> +++ b/src/systemctl/systemctl.c
> @@ -5290,6 +5290,95 @@ finish:
> return r;
> }
>
> +static int add_install_dependency(sd_bus *bus, char **args) {
> + _cleanup_strv_free_ char **names = NULL;
> + _cleanup_free_ char *target = NULL;
> + const char *verb = args[0];
> + UnitFileInstallDependency dep;
> + int r = 0;
> +
> + if (!args[1])
> + return 0;
> +
> + target = unit_name_mangle_with_suffix(args[1], MANGLE_NOGLOB, ".target");
> + if (!target)
> + return log_oom();
> +
> + r = mangle_names(args+2, &names);
> + if (r < 0)
> + return r;
> +
> + if (streq(verb, "add-wants"))
> + dep = UNIT_FILE_INSTALL_DEPENDENCY_WANTS;
> + else if (streq(verb, "add-requires"))
> + dep = UNIT_FILE_INSTALL_DEPENDENCY_REQUIRES;
> + else
> + assert_not_reached("Unknown verb");
> +
> + if (!bus || avoid_bus()) {
> + UnitFileChange *changes = NULL;
> + unsigned n_changes = 0;
> +
> + r = unit_file_add_install_dependency(arg_scope, arg_runtime, arg_root, names, target, dep, arg_force, &changes, &n_changes);
> +
> + if (!arg_quiet)
> + dump_unit_file_changes(changes, n_changes);
> +
> + unit_file_changes_free(changes, n_changes);
> +
> + } else {
> + _cleanup_bus_message_unref_ sd_bus_message *reply = NULL, *m = NULL;
> + _cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
> +
> + r = sd_bus_message_new_method_call(
> + bus,
> + &m,
> + "org.freedesktop.systemd1",
> + "/org/freedesktop/systemd1",
> + "org.freedesktop.systemd1.Manager",
> + "AddInstallDependencyUnitFiles");
> + if (r < 0)
> + return bus_log_create_error(r);
> +
> + r = sd_bus_message_append_strv(m, names);
> + if (r < 0)
> + return bus_log_create_error(r);
> +
> + r = sd_bus_message_append(m, "s", target);
> + if (r < 0)
> + return bus_log_create_error(r);
> +
> + r = sd_bus_message_append(m, "s", unit_file_install_dependency_to_string(dep));
> + if (r < 0)
> + return bus_log_create_error(r);
> +
> + r = sd_bus_message_append(m, "b", arg_runtime);
> + if (r < 0)
> + return bus_log_create_error(r);
> +
> + r = sd_bus_message_append(m, "b", arg_force);
> + if (r < 0)
> + return bus_log_create_error(r);
> +
> + r = sd_bus_call(bus, m, 0, &error, &reply);
> + if (r < 0) {
> + log_error("Failed to execute operation: %s", bus_error_message(&error, r));
> + return r;
> + }
> +
> + r = deserialize_and_dump_unit_file_changes(reply);
> + if (r < 0)
> + return r;
> +
> + if (!arg_no_reload)
> + r = daemon_reload(bus, args);
> + else
> + r = 0;
> + }
> +
> + return r;
> +}
> +
> static int preset_all(sd_bus *bus, char **args) {
> UnitFileChange *changes = NULL;
> unsigned n_changes = 0;
> @@ -5535,6 +5624,11 @@ static void systemctl_help(void) {
> " unmask NAME... Unmask one or more units\n"
> " link PATH... Link one or more units files into\n"
> " the search path\n"
> + " based on preset configuration\n"
Extra line now? ;)
> + " add-wants TARGET NAME... Add 'Wants' dependency for the target\n"
> + " on specified one or more units\n"
> + " add-requires TARGET NAME... Add 'Requires' dependency for the target\n"
> + " on specified one or more units\n"
> " get-default Get the name of the default target\n"
> " set-default NAME Set the default target\n\n"
> "Machine Commands:\n"
> @@ -6545,6 +6639,8 @@ static int systemctl_main(sd_bus *bus, int argc, char *argv[], int bus_error) {
> { "get-default", EQUAL, 1, get_default, NOBUS },
> { "set-property", MORE, 3, set_property },
> { "is-system-running", EQUAL, 1, is_system_running },
> + { "add-wants", MORE, 3, add_install_dependency, NOBUS },
> + { "add-requires", MORE, 3, add_install_dependency, NOBUS },
> {}
> }, *verb = verbs;
Zbyszek
More information about the systemd-devel
mailing list