[systemd-devel] [PATCH] core/cryptsetup: Add WantsMountFor option to enable fallback to password request for crypt mounts. Signed-off-by: Przemek Rudy <prudy1 at o2.pl>

[Przemyslaw Rudy]

Hi Lennart,
inline...

On 04/28/2014 04:31 PM, Lennart Poettering wrote:
> On Sun, 27.04.14 23:46, Przemek Rudy (prudy1 at 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