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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel