Hi Tom, Sure, I'll get rid of this signed-off soon and re-send. It has been tested with fedora 20 for all three options: - with device inserted - with device removed during startup but inserted before the mount timeout - with device removed
Thanks Przemek On 04/28/2014 10:15 AM, Tom Gundersen wrote: > On Mon, Apr 28, 2014 at 12:46 AM, Przemek Rudy <[email protected]> wrote: >> This patch is a proposal for a problem with not falling back to password >> request >> if the device with unlocking key for crypt volumes is not mounted for >> defined time. > > Looks good to me (but I didn't test it). Only one minor nit below. > > Also, you probably want to rewrite the subject of the patch (you can > drop the signed-off-by as we don't use that here). > > Cheers, > > Tom > >> --- >> src/core/dbus-unit.c | 1 + >> src/core/load-fragment-gperf.gperf.m4 | 1 + >> src/core/load-fragment.c | 47 +++++++++++ >> src/core/load-fragment.h | 1 + >> src/core/manager.c | 15 ++++ >> src/core/manager.h | 6 ++ >> src/core/mount.c | 25 +++++- >> src/core/unit.c | 144 >> ++++++++++++++++++++++++++++++++++ >> src/core/unit.h | 5 ++ >> src/cryptsetup/cryptsetup-generator.c | 3 +- >> 10 files changed, 246 insertions(+), 2 deletions(-) >> >> diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c >> index 07e7f20..31a35e9 100644 >> --- a/src/core/dbus-unit.c >> +++ b/src/core/dbus-unit.c >> @@ -516,6 +516,7 @@ const sd_bus_vtable bus_unit_vtable[] = { >> SD_BUS_PROPERTY("ReloadPropagatedFrom", "as", >> property_get_dependencies, offsetof(Unit, >> dependencies[UNIT_RELOAD_PROPAGATED_FROM]), SD_BUS_VTABLE_PROPERTY_CONST), >> SD_BUS_PROPERTY("JoinsNamespaceOf", "as", >> property_get_dependencies, offsetof(Unit, >> dependencies[UNIT_JOINS_NAMESPACE_OF]), SD_BUS_VTABLE_PROPERTY_CONST), >> SD_BUS_PROPERTY("RequiresMountsFor", "as", NULL, offsetof(Unit, >> requires_mounts_for), SD_BUS_VTABLE_PROPERTY_CONST), >> + SD_BUS_PROPERTY("WantsMountsFor", "as", NULL, offsetof(Unit, >> wants_mounts_for), SD_BUS_VTABLE_PROPERTY_CONST), >> SD_BUS_PROPERTY("Documentation", "as", NULL, offsetof(Unit, >> documentation), SD_BUS_VTABLE_PROPERTY_CONST), >> SD_BUS_PROPERTY("Description", "s", property_get_description, 0, >> SD_BUS_VTABLE_PROPERTY_CONST), >> SD_BUS_PROPERTY("LoadState", "s", property_get_load_state, >> offsetof(Unit, load_state), SD_BUS_VTABLE_PROPERTY_CONST), >> diff --git a/src/core/load-fragment-gperf.gperf.m4 >> b/src/core/load-fragment-gperf.gperf.m4 >> index 21bccbb..4e51866 100644 >> --- a/src/core/load-fragment-gperf.gperf.m4 >> +++ b/src/core/load-fragment-gperf.gperf.m4 >> @@ -139,6 +139,7 @@ Unit.PropagateReloadFrom, config_parse_unit_deps, >> UNIT_RELOAD >> Unit.PartOf, config_parse_unit_deps, >> UNIT_PART_OF, 0 >> Unit.JoinsNamespaceOf, config_parse_unit_deps, >> UNIT_JOINS_NAMESPACE_OF, 0 >> Unit.RequiresMountsFor, config_parse_unit_requires_mounts_for, 0, >> 0 >> +Unit.WantsMountsFor, config_parse_unit_wants_mounts_for, 0, >> 0 >> Unit.StopWhenUnneeded, config_parse_bool, 0, >> offsetof(Unit, stop_when_unneeded) >> Unit.RefuseManualStart, config_parse_bool, 0, >> offsetof(Unit, refuse_manual_start) >> Unit.RefuseManualStop, config_parse_bool, 0, >> offsetof(Unit, refuse_manual_stop) >> diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c >> index 3b36d15..24c1849 100644 >> --- a/src/core/load-fragment.c >> +++ b/src/core/load-fragment.c >> @@ -2048,6 +2048,52 @@ int config_parse_unit_requires_mounts_for( >> return 0; >> } >> >> +int config_parse_unit_wants_mounts_for( >> + const char *unit, >> + const char *filename, >> + unsigned line, >> + const char *section, >> + unsigned section_line, >> + const char *lvalue, >> + int ltype, >> + const char *rvalue, >> + void *data, >> + void *userdata) { >> + >> + Unit *u = userdata; >> + char *state; >> + size_t l; >> + char *w; >> + >> + assert(filename); >> + assert(lvalue); >> + assert(rvalue); >> + assert(data); >> + >> + FOREACH_WORD_QUOTED(w, l, rvalue, state) { >> + int r; >> + _cleanup_free_ char *n; >> + >> + n = strndup(w, l); >> + if (!n) >> + return log_oom(); >> + >> + if (!utf8_is_valid(n)) { >> + log_invalid_utf8(unit, LOG_ERR, filename, line, >> EINVAL, rvalue); >> + continue; >> + } >> + >> + r = unit_want_mounts_for(u, n); >> + if (r < 0) { >> + log_syntax(unit, LOG_ERR, filename, line, r, >> + "Failed to add wanted mount for, >> ignoring: %s", rvalue); >> + continue; >> + } >> + } >> + >> + return 0; >> +} >> + >> int config_parse_documentation(const char *unit, >> const char *filename, >> unsigned line, >> @@ -3422,6 +3468,7 @@ void unit_dump_config_items(FILE *f) { >> { config_parse_nsec, "NANOSECONDS" }, >> { config_parse_namespace_path_strv, "PATH [...]" }, >> { config_parse_unit_requires_mounts_for, "PATH [...]" }, >> + { config_parse_unit_wants_mounts_for, "PATH [...]" }, >> { config_parse_exec_mount_flags, "MOUNTFLAG [...]" }, >> { config_parse_unit_string_printf, "STRING" }, >> { config_parse_trigger_unit, "UNIT" }, >> diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h >> index 242fd27..761d936 100644 >> --- a/src/core/load-fragment.h >> +++ b/src/core/load-fragment.h >> @@ -74,6 +74,7 @@ int config_parse_kill_mode(const char *unit, const char >> *filename, unsigned line >> int config_parse_notify_access(const char *unit, const char *filename, >> unsigned line, const char *section, unsigned section_line, const char >> *lvalue, int ltype, const char *rvalue, void *data, void *userdata); >> int config_parse_failure_action(const char *unit, const char *filename, >> unsigned line, const char *section, unsigned section_line, const char >> *lvalue, int ltype, const char *rvalue, void *data, void *userdata); >> int config_parse_unit_requires_mounts_for(const char *unit, const char >> *filename, unsigned line, const char *section, unsigned section_line, const >> char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); >> +int config_parse_unit_wants_mounts_for(const char *unit, const char >> *filename, unsigned line, const char *section, unsigned section_line, const >> char *lvalue, int ltype, const char *rvalue, void *data, void *userdata); >> int config_parse_syscall_filter(const char *unit, const char *filename, >> unsigned line, const char *section, unsigned section_line, const char >> *lvalue, int ltype, const char *rvalue, void *data, void *userdata); >> int config_parse_syscall_archs(const char *unit, const char *filename, >> unsigned line, const char *section, unsigned section_line, const char >> *lvalue, int ltype, const char *rvalue, void *data, void *userdata); >> int config_parse_syscall_errno(const char *unit, const char *filename, >> unsigned line, const char *section, unsigned section_line, const char >> *lvalue, int ltype, const char *rvalue, void *data, void *userdata); >> diff --git a/src/core/manager.c b/src/core/manager.c >> index 5772f40..84ce11d 100644 >> --- a/src/core/manager.c >> +++ b/src/core/manager.c >> @@ -830,6 +830,9 @@ void manager_free(Manager *m) { >> assert(hashmap_isempty(m->units_requiring_mounts_for)); >> hashmap_free(m->units_requiring_mounts_for); >> >> + assert(hashmap_isempty(m->units_want_mounts_for)); >> + hashmap_free(m->units_want_mounts_for); >> + >> free(m); >> } >> >> @@ -2849,6 +2852,18 @@ Set *manager_get_units_requiring_mounts_for(Manager >> *m, const char *path) { >> return hashmap_get(m->units_requiring_mounts_for, streq(p, "/") ? >> "" : p); >> } >> >> +Set *manager_get_units_want_mounts_for(Manager *m, const char *path) { >> + char p[strlen(path)+1]; >> + >> + assert(m); >> + assert(path); >> + >> + strcpy(p, path); >> + path_kill_slashes(p); >> + >> + return hashmap_get(m->units_want_mounts_for, streq(p, "/") ? "" : >> p); >> +} >> + >> const char *manager_get_runtime_prefix(Manager *m) { >> assert(m); >> >> diff --git a/src/core/manager.h b/src/core/manager.h >> index a3de351..ba5e688 100644 >> --- a/src/core/manager.h >> +++ b/src/core/manager.h >> @@ -269,6 +269,11 @@ struct Manager { >> * value where Unit objects are contained. */ >> Hashmap *units_requiring_mounts_for; >> >> + /* This maps all possible path prefixes to the units want >> + * them. It's a hashmap with a path string as key and a Set as >> + * value where Unit objects are contained. */ >> + Hashmap *units_want_mounts_for; > > Shouldn't this be "units_wanting_mounts_for" to mimic > "units_requiring_mounts_for"? > >> /* Reference to the kdbus bus control fd */ >> int kdbus_fd; >> }; >> @@ -335,6 +340,7 @@ void manager_status_printf(Manager *m, bool ephemeral, >> const char *status, const >> void manager_flip_auto_status(Manager *m, bool enable); >> >> Set *manager_get_units_requiring_mounts_for(Manager *m, const char *path); >> +Set *manager_get_units_want_mounts_for(Manager *m, const char *path); >> >> const char *manager_get_runtime_prefix(Manager *m); >> >> diff --git a/src/core/mount.c b/src/core/mount.c >> index a979837..64d6914 100644 >> --- a/src/core/mount.c >> +++ b/src/core/mount.c >> @@ -264,7 +264,7 @@ static int mount_add_mount_links(Mount *m) { >> return r; >> } >> >> - /* Adds in links to other units that use this path or paths >> + /* Adds in links to other units that use (require) this path or >> paths >> * further down in the hierarchy */ >> s = manager_get_units_requiring_mounts_for(UNIT(m)->manager, >> m->where); >> SET_FOREACH(other, s, i) { >> @@ -287,6 +287,29 @@ static int mount_add_mount_links(Mount *m) { >> } >> } >> >> + /* Adds in links to other units that use (want) this path or paths >> + * further down in the hierarchy */ >> + s = manager_get_units_want_mounts_for(UNIT(m)->manager, m->where); >> + SET_FOREACH(other, s, i) { >> + >> + if (other->load_state != UNIT_LOADED) >> + continue; >> + >> + if (other == UNIT(m)) >> + continue; >> + >> + r = unit_add_dependency(other, UNIT_AFTER, UNIT(m), true); >> + if (r < 0) >> + return r; >> + >> + if (UNIT(m)->fragment_path) { >> + /* If we have fragment configuration, then make >> this dependency required */ >> + r = unit_add_dependency(other, UNIT_WANTS, UNIT(m), >> true); >> + if (r < 0) >> + return r; >> + } >> + } >> + >> return 0; >> } >> >> diff --git a/src/core/unit.c b/src/core/unit.c >> index 6ac359e..f7c03f7 100644 >> --- a/src/core/unit.c >> +++ b/src/core/unit.c >> @@ -428,6 +428,34 @@ static void unit_free_requires_mounts_for(Unit *u) { >> u->requires_mounts_for = NULL; >> } >> >> +static void unit_free_wants_mounts_for(Unit *u) { >> + char **j; >> + >> + STRV_FOREACH(j, u->wants_mounts_for) { >> + char s[strlen(*j) + 1]; >> + >> + PATH_FOREACH_PREFIX_MORE(s, *j) { >> + char *y; >> + Set *x; >> + >> + x = hashmap_get2(u->manager->units_want_mounts_for, >> s, (void**) &y); >> + if (!x) >> + continue; >> + >> + set_remove(x, u); >> + >> + if (set_isempty(x)) { >> + >> hashmap_remove(u->manager->units_want_mounts_for, y); >> + free(y); >> + set_free(x); >> + } >> + } >> + } >> + >> + strv_free(u->wants_mounts_for); >> + u->wants_mounts_for = NULL; >> +} >> + >> static void unit_done(Unit *u) { >> ExecContext *ec; >> CGroupContext *cc; >> @@ -464,6 +492,7 @@ void unit_free(Unit *u) { >> unit_done(u); >> >> unit_free_requires_mounts_for(u); >> + unit_free_wants_mounts_for(u); >> >> SET_FOREACH(t, u->names, i) >> hashmap_remove_value(u->manager->units, t, u); >> @@ -885,6 +914,16 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { >> fputs("\n", f); >> } >> >> + if (!strv_isempty(u->wants_mounts_for)) { >> + fprintf(f, >> + "%s\tWantsMountsFor:", prefix); >> + >> + STRV_FOREACH(j, u->wants_mounts_for) >> + fprintf(f, " %s", *j); >> + >> + fputs("\n", f); >> + } >> + >> if (u->load_state == UNIT_LOADED) { >> >> fprintf(f, >> @@ -1068,6 +1107,35 @@ static int unit_add_mount_dependencies(Unit *u) { >> } >> } >> >> + STRV_FOREACH(i, u->wants_mounts_for) { >> + char prefix[strlen(*i) + 1]; >> + >> + PATH_FOREACH_PREFIX_MORE(prefix, *i) { >> + Unit *m; >> + >> + r = manager_get_unit_by_path(u->manager, prefix, >> ".mount", &m); >> + if (r < 0) >> + return r; >> + if (r == 0) >> + continue; >> + if (m == u) >> + continue; >> + >> + if (m->load_state != UNIT_LOADED) >> + continue; >> + >> + r = unit_add_dependency(u, UNIT_AFTER, m, true); >> + if (r < 0) >> + return r; >> + >> + if (m->fragment_path) { >> + r = unit_add_dependency(u, UNIT_WANTS, m, >> true); >> + if (r < 0) >> + return r; >> + } >> + } >> + } >> + >> return 0; >> } >> >> @@ -3289,6 +3357,82 @@ int unit_require_mounts_for(Unit *u, const char >> *path) { >> return 0; >> } >> >> +int unit_want_mounts_for(Unit *u, const char *path) { >> + char prefix[strlen(path) + 1], *p; >> + int r; >> + >> + assert(u); >> + assert(path); >> + >> + /* Registers a unit for want a certain path and all its >> + * prefixes. We keep a simple array of these paths in the >> + * unit, since its usually short. However, we build a prefix >> + * table for all possible prefixes so that new appearing mount >> + * units can easily determine which units to make themselves a >> + * dependency of. */ >> + >> + if (!path_is_absolute(path)) >> + return -EINVAL; >> + >> + p = strdup(path); >> + if (!p) >> + return -ENOMEM; >> + >> + path_kill_slashes(p); >> + >> + if (!path_is_safe(p)) { >> + free(p); >> + return -EPERM; >> + } >> + >> + if (strv_contains(u->wants_mounts_for, p)) { >> + free(p); >> + return 0; >> + } >> + >> + r = strv_consume(&u->wants_mounts_for, p); >> + if (r < 0) >> + return r; >> + >> + PATH_FOREACH_PREFIX_MORE(prefix, p) { >> + Set *x; >> + >> + x = hashmap_get(u->manager->units_want_mounts_for, prefix); >> + if (!x) { >> + char *q; >> + >> + if (!u->manager->units_want_mounts_for) { >> + u->manager->units_want_mounts_for = >> hashmap_new(string_hash_func, string_compare_func); >> + if (!u->manager->units_want_mounts_for) >> + return -ENOMEM; >> + } >> + >> + q = strdup(prefix); >> + if (!q) >> + return -ENOMEM; >> + >> + x = set_new(NULL, NULL); >> + if (!x) { >> + free(q); >> + return -ENOMEM; >> + } >> + >> + r = hashmap_put(u->manager->units_want_mounts_for, >> q, x); >> + if (r < 0) { >> + free(q); >> + set_free(x); >> + return r; >> + } >> + } >> + >> + r = set_put(x, u); >> + if (r < 0) >> + return r; >> + } >> + >> + return 0; >> +} >> + >> int unit_setup_exec_runtime(Unit *u) { >> ExecRuntime **rt; >> size_t offset; >> diff --git a/src/core/unit.h b/src/core/unit.h >> index 3e61067..0db0b91 100644 >> --- a/src/core/unit.h >> +++ b/src/core/unit.h >> @@ -143,6 +143,7 @@ struct Unit { >> Set *dependencies[_UNIT_DEPENDENCY_MAX]; >> >> char **requires_mounts_for; >> + char **wants_mounts_for; >> >> char *description; >> char **documentation; >> @@ -189,6 +190,9 @@ struct Unit { >> /* All units which have requires_mounts_for set */ >> LIST_FIELDS(Unit, has_requires_mounts_for); >> >> + /* All units which have wantss_mounts_for set */ >> + LIST_FIELDS(Unit, has_wants_mounts_for); >> + >> /* Load queue */ >> LIST_FIELDS(Unit, load_queue); >> >> @@ -625,6 +629,7 @@ int unit_kill_context(Unit *u, KillContext *c, bool >> sigkill, pid_t main_pid, pid >> int unit_make_transient(Unit *u); >> >> int unit_require_mounts_for(Unit *u, const char *path); >> +int unit_want_mounts_for(Unit *u, const char *path); >> >> const char *unit_active_state_to_string(UnitActiveState i) _const_; >> UnitActiveState unit_active_state_from_string(const char *s) _pure_; >> diff --git a/src/cryptsetup/cryptsetup-generator.c >> b/src/cryptsetup/cryptsetup-generator.c >> index f4eeb2a..ce7ae92 100644 >> --- a/src/cryptsetup/cryptsetup-generator.c >> +++ b/src/cryptsetup/cryptsetup-generator.c >> @@ -153,7 +153,8 @@ static int create_disk( >> >> fprintf(f, "After=%1$s\nRequires=%1$s\n", >> dd); >> } else >> - fprintf(f, "RequiresMountsFor=%s\n", >> password); >> + /* Do not use 'RequiresMountsFor=' here to >> allow fallback to password in case the key device is not available */ >> + fprintf(f, "WantsMountsFor=%s\n", password); >> } >> } >> >> -- >> 1.9.0 >> >> _______________________________________________ >> systemd-devel mailing list >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel > _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
