Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts.
On Sun, 25.05.14 12:39, Przemek Rudy (pru...@o2.pl) wrote:
-Set *manager_get_units_requiring_mounts_for(Manager *m, const char *path) {
+Set *manager_get_units_need_mounts_for(Manager *m, const char *path,
bool strong) {
Please don't invent new bools halfway. Please always use the same logic
here to discern the two kinds, i.e. the dep type enum.
* them. It's a hashmap with a path string as key and a Set as
- * value where Unit objects are contained. */
-Hashmap *units_requiring_mounts_for;
+ * value where Unit objects are contained.
+ * [0] - map of required (strong) paths
+ * [1] - map of wanted (weak) paths */
+Hashmap *units_need_mounts_for[2];
Please use two normal variables for this. Two isn't that many that you'd
need to defer to an array for this. Also, an array where we use
arbitrary numeric indexes for reference two different concepts is not
OK. If we do this, then we must have an enum to reference this. Also,
introducing a new numbering scheme, where we already have the dep type
as enum...
Generally, I still don't really feel that the usecase for this is strong
enough. Can you make a strong concise case why we want this? I am not
totally opposed, but I want to know why I merge this...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts.
On 21.05.2014 10:03, Lennart Poettering wrote:
On Sun, 18.05.14 19:36, Przemek Rudy (pru...@o2.pl) wrote:
diff --git a/src/core/automount.c b/src/core/automount.c
index 65e6d6f..d4359b9 100644
--- a/src/core/automount.c
+++ b/src/core/automount.c
@@ -124,7 +124,7 @@ static int automount_add_mount_links(Automount *a) {
if (r 0)
return r;
-return unit_require_mounts_for(UNIT(a), parent);
+return unit_needs_mounts_for(UNIT(a), parent, true);
}
Please make the the last parameter one of type UnitDependency, so that
one can either pass UNIT_WANTS or UNIT_REQUIRES there. THat makes the
calls much more descriptive, as well as more generic.
Otherwise looks pretty good!
Lennart
Hmm, I might see a problem in the initramfs, where devices show up bit by bit.
The user might then be confronted with the password dialog, although the device
with the key file will show up a little bit later.
But I might be wrong. Time will tell.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts.
On 05/27/2014 01:37 PM, Harald Hoyer wrote:
On 21.05.2014 10:03, Lennart Poettering wrote:
On Sun, 18.05.14 19:36, Przemek Rudy (pru...@o2.pl) wrote:
diff --git a/src/core/automount.c b/src/core/automount.c
index 65e6d6f..d4359b9 100644
--- a/src/core/automount.c
+++ b/src/core/automount.c
@@ -124,7 +124,7 @@ static int automount_add_mount_links(Automount *a) {
if (r 0)
return r;
-return unit_require_mounts_for(UNIT(a), parent);
+return unit_needs_mounts_for(UNIT(a), parent, true);
}
Please make the the last parameter one of type UnitDependency, so that
one can either pass UNIT_WANTS or UNIT_REQUIRES there. THat makes the
calls much more descriptive, as well as more generic.
Otherwise looks pretty good!
Lennart
Hmm, I might see a problem in the initramfs, where devices show up bit by bit.
The user might then be confronted with the password dialog, although the
device
with the key file will show up a little bit later.
But I might be wrong. Time will tell.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Hello Harald,
After years, same thing different place, was in dracut now in systemd. :)
Please note that the key device has its own mounting timeout that goes into
fstab:
x-systemd.device-timeout=10,auto,nofail
this ensures we have x-systemd.device-timeout seconds to wait for the key
device before give up.
Best,
Przemek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts.
Updated using 'UNIT_REQUIRES' instead 'true'.
---
src/core/automount.c | 2 +-
src/core/dbus-unit.c | 3 +-
src/core/load-fragment-gperf.gperf.m4 | 3 +-
src/core/load-fragment.c | 8 +--
src/core/load-fragment.h | 2 +-
src/core/manager.c| 11 ++--
src/core/manager.h| 8 ++-
src/core/mount.c | 40 --
src/core/path.c | 2 +-
src/core/socket.c | 2 +-
src/core/swap.c | 2 +-
src/core/timer.c | 2 +-
src/core/unit.c | 101 --
src/core/unit.h | 7 +--
src/cryptsetup/cryptsetup-generator.c | 3 +-
15 files changed, 111 insertions(+), 85 deletions(-)
diff --git a/src/core/automount.c b/src/core/automount.c
index 65e6d6f..6cfe7b6 100644
--- a/src/core/automount.c
+++ b/src/core/automount.c
@@ -124,7 +124,7 @@ static int automount_add_mount_links(Automount *a) {
if (r 0)
return r;
-return unit_require_mounts_for(UNIT(a), parent);
+return unit_needs_mounts_for(UNIT(a), parent, UNIT_REQUIRES);
}
static int automount_add_default_dependencies(Automount *a) {
diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c
index 07e7f20..18d21ff 100644
--- a/src/core/dbus-unit.c
+++ b/src/core/dbus-unit.c
@@ -515,7 +515,8 @@ const sd_bus_vtable bus_unit_vtable[] = {
SD_BUS_PROPERTY(PropagatesReloadTo, as, property_get_dependencies,
offsetof(Unit, dependencies[UNIT_PROPAGATES_RELOAD_TO]),
SD_BUS_VTABLE_PROPERTY_CONST),
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(RequiresMountsFor, as, NULL, offsetof(Unit,
needs_mounts_for[0]), SD_BUS_VTABLE_PROPERTY_CONST),
+SD_BUS_PROPERTY(WantsMountsFor, as, NULL, offsetof(Unit,
needs_mounts_for[1]), 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..7b9406b 100644
--- a/src/core/load-fragment-gperf.gperf.m4
+++ b/src/core/load-fragment-gperf.gperf.m4
@@ -138,7 +138,8 @@ Unit.ReloadPropagatedFrom, config_parse_unit_deps,
UNIT_RELOAD
Unit.PropagateReloadFrom,config_parse_unit_deps,
UNIT_RELOAD_PROPAGATED_FROM, 0
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.RequiresMountsFor, config_parse_unit_needs_mounts_for,
UNIT_REQUIRES, 0
+Unit.WantsMountsFor, config_parse_unit_needs_mounts_for,
UNIT_WANTS,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..aadc930 100644
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -2002,7 +2002,7 @@ int config_parse_unit_condition_null(const char *unit,
DEFINE_CONFIG_PARSE_ENUM(config_parse_notify_access, notify_access,
NotifyAccess, Failed to parse notify access specifier);
DEFINE_CONFIG_PARSE_ENUM(config_parse_failure_action, failure_action,
FailureAction, Failed to parse failure action specifier);
-int config_parse_unit_requires_mounts_for(
+int config_parse_unit_needs_mounts_for(
const char *unit,
const char *filename,
unsigned line,
@@ -2037,10 +2037,10 @@ int config_parse_unit_requires_mounts_for(
continue;
}
-r = unit_require_mounts_for(u, n);
+r = unit_needs_mounts_for(u, n,
Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts.
On Sun, 18.05.14 19:36, Przemek Rudy (pru...@o2.pl) wrote:
diff --git a/src/core/automount.c b/src/core/automount.c
index 65e6d6f..d4359b9 100644
--- a/src/core/automount.c
+++ b/src/core/automount.c
@@ -124,7 +124,7 @@ static int automount_add_mount_links(Automount *a) {
if (r 0)
return r;
-return unit_require_mounts_for(UNIT(a), parent);
+return unit_needs_mounts_for(UNIT(a), parent, true);
}
Please make the the last parameter one of type UnitDependency, so that
one can either pass UNIT_WANTS or UNIT_REQUIRES there. THat makes the
calls much more descriptive, as well as more generic.
Otherwise looks pretty good!
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts.
Combined into one ptach.
I see it is not clear how this patch is related to the cryptsetup password
timeout. Note the cryptsetup timeout is done by the cryptsetup service. Thus it
will occur only if the cryptsetup is reached and the key is not valid. But that
is true only for the key file on accessible path. When the key is on removable
device and the device is not inserted the cryptsetup will not even be started.
And no any cryptsetup timeout has a chance to show itself. This is because of a
strong rule 'RequiresMountsFor'. If the device is not present, the requirement
fails preventing cryptsetup service from being started. The cryptsetup does not
really 'require' the device with key on it, it only 'wants' this device, so it
can start at all with the fallback to password request. This patch is changing
the 'requires' into 'wants' allowing cryptsetup service startup. It is already
covered by ancient bug in bugzilla redhat #905683.
---
src/core/automount.c | 2 +-
src/core/dbus-unit.c | 3 +-
src/core/load-fragment-gperf.gperf.m4 | 3 +-
src/core/load-fragment.c | 8 +--
src/core/load-fragment.h | 2 +-
src/core/manager.c| 11 ++--
src/core/manager.h| 8 +--
src/core/mount.c | 40 +++---
src/core/path.c | 2 +-
src/core/socket.c | 2 +-
src/core/swap.c | 2 +-
src/core/timer.c | 2 +-
src/core/unit.c | 99 ---
src/core/unit.h | 7 +--
src/cryptsetup/cryptsetup-generator.c | 3 +-
15 files changed, 109 insertions(+), 85 deletions(-)
diff --git a/src/core/automount.c b/src/core/automount.c
index 65e6d6f..d4359b9 100644
--- a/src/core/automount.c
+++ b/src/core/automount.c
@@ -124,7 +124,7 @@ static int automount_add_mount_links(Automount *a) {
if (r 0)
return r;
-return unit_require_mounts_for(UNIT(a), parent);
+return unit_needs_mounts_for(UNIT(a), parent, true);
}
static int automount_add_default_dependencies(Automount *a) {
diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c
index 07e7f20..18d21ff 100644
--- a/src/core/dbus-unit.c
+++ b/src/core/dbus-unit.c
@@ -515,7 +515,8 @@ const sd_bus_vtable bus_unit_vtable[] = {
SD_BUS_PROPERTY(PropagatesReloadTo, as, property_get_dependencies,
offsetof(Unit, dependencies[UNIT_PROPAGATES_RELOAD_TO]),
SD_BUS_VTABLE_PROPERTY_CONST),
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(RequiresMountsFor, as, NULL, offsetof(Unit,
needs_mounts_for[0]), SD_BUS_VTABLE_PROPERTY_CONST),
+SD_BUS_PROPERTY(WantsMountsFor, as, NULL, offsetof(Unit,
needs_mounts_for[1]), 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..7b9406b 100644
--- a/src/core/load-fragment-gperf.gperf.m4
+++ b/src/core/load-fragment-gperf.gperf.m4
@@ -138,7 +138,8 @@ Unit.ReloadPropagatedFrom, config_parse_unit_deps,
UNIT_RELOAD
Unit.PropagateReloadFrom,config_parse_unit_deps,
UNIT_RELOAD_PROPAGATED_FROM, 0
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.RequiresMountsFor, config_parse_unit_needs_mounts_for,
UNIT_REQUIRES, 0
+Unit.WantsMountsFor, config_parse_unit_needs_mounts_for,
UNIT_WANTS,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
Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts. Signed-off-by: Przemek Rudy pru...@o2.pl
On Mon, 28.04.14 17:22, Przemyslaw Rudy (pru...@o2.pl) 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.
Can you elaborate on the usecase? I mean, this would still result in in
90s timeout, right? Or what's your idea here?
This does not change any cryptsetup timeout. It simply allows using it.
As when the key device is not in place the cryptsetup is not started at
all and thus its internal timeout does not work either.
Not sure I follow here.. I mean, if it's listed in crypttab, and pulled
in via Wants or Requires then a .device job will be queued for the
device. And we weill wait for it, up to 90s. After those 90s we will go
on, and hence things won't totally fail.
Are you saying that you are OK with waiting the 90s until you get the
password prompt?
+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) {
+}
Sounds like a call to unify with
config_parse_unit_requires_mounts_for(), and use the ltype param to
distuingish them.
Yes, duplicated code. However still the question what is worth more -
duplicate one loop or add a parameter change body adding two if-f and
then replace all original calls to original function...
I do not insists on any, keep this for purists.
Yes, no duplicate code please... And instead of having large if blocks,
just determine the variables you need to operate on, and then run the
same code on those variables...
Totally not funny anymore...
Same answer to all, duplicate few loops or do more changes?
So do I understand correctly, you want this to be funny somehow? What
are the preferences tehn, using parameter to all common functions?
I'll wait anyway few days for any more comments on this, if no more I
can do the changes.
Sorry for not responding more timely.
Yeah, I am not a fan of duplicated code. Please try to figure out the
params early, and then operate on that, with common code, if you follow
what I mean...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts. Signed-off-by: Przemek Rudy pru...@o2.pl
On Mon, Apr 28, 2014 at 12:46 AM, Przemek Rudy pru...@o2.pl 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
Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts. Signed-off-by: Przemek Rudy pru...@o2.pl
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 pru...@o2.pl 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);
+
Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts. Signed-off-by: Przemek Rudy pru...@o2.pl
On Sun, 27.04.14 23:46, Przemek Rudy (pru...@o2.pl) 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.
Can you elaborate on the usecase? I mean, this would still result in in
90s timeout, right? Or what's your idea here?
+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) {
+}
Sounds like a call to unify with
config_parse_unit_requires_mounts_for(), and use the ltype param to
distuingish them.
+/* 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;
+}
+}
+
Something to unify with the requires codepath. For example, it should
be easy to turn most of the loop's body into a function of its own, that
could then be reused...
+static void unit_free_wants_mounts_for(Unit *u) {
Same here..
Please, let's not let turn this into a game of copy/paste...
+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);
+}
+
Same here...
+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;
+}
+}
+}
+
Not funny anymore.
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) {
Totally not funny anymore...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts. Signed-off-by: Przemek Rudy pru...@o2.pl
Hi Lennart,
inline...
On 04/28/2014 04:31 PM, Lennart Poettering wrote:
On Sun, 27.04.14 23:46, Przemek Rudy (pru...@o2.pl) 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.
Can you elaborate on the usecase? I mean, this would still result in in
90s timeout, right? Or what's your idea here?
This does not change any cryptsetup timeout. It simply allows using it.
As when the key device is not in place the cryptsetup is not started at
all and thus its internal timeout does not work either.
+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) {
+}
Sounds like a call to unify with
config_parse_unit_requires_mounts_for(), and use the ltype param to
distuingish them.
Yes, duplicated code. However still the question what is worth more -
duplicate one loop or add a parameter change body adding two if-f and
then replace all original calls to original function...
I do not insists on any, keep this for purists.
+/* 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;
+}
+}
+
Something to unify with the requires codepath. For example, it should
be easy to turn most of the loop's body into a function of its own, that
could then be reused...
+static void unit_free_wants_mounts_for(Unit *u) {
Same here..
Please, let's not let turn this into a game of copy/paste...
+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);
+}
+
Same here...
+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;
+}
+}
+}
+
Not funny anymore.
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) {
Totally not funny anymore...
Same answer to all, duplicate few loops or do more changes?
So do I understand correctly, you want this to be funny somehow? What
are the preferences tehn, using parameter to all common functions?
I'll wait anyway few days for any more comments on this, if no more I
can do the changes.
Lennart
Thanks
Przemek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts. Signed-off-by: Przemek Rudy pru...@o2.pl
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.
---
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
