[systemd-devel] [PATCH] sysctl: don't replace dots with slashes in prefix

[Jan Synacek]

David Herrmann <dh.herrmann at gmail.com> writes:
> Hi
>
> On Mon, Sep 15, 2014 at 1:22 PM, Jan Synacek <jsynacek at redhat.com> wrote:
>> David Herrmann <dh.herrmann at gmail.com> writes:
>>> A path isn't necessarily a file-system path. With sysctl, we have to
>>> ways to specify entries:
>>>
>>>   1) You can specify them via legacy sysctl(2) names. These names use
>>> dots as separators
>>>
>>>   2) You can specify them via /proc paths. These use slashes as separators.
>>>
>>> The conversion between 1) and 2) (both ways) replaces dots by slashes
>>> and slashes by dots. However, you *always* have to convert both! That
>>> is, if a sysctl name is "root.some.weird/entry", then this becomes
>>> "root/some/weird.entry".
>>>
>>> Now, the systemd-sysctl --prefix argument says it takes "paths",
>>> however, it replaces dots with slashes, which made me assume it takes
>>> legacy names instead. Nevertheless, this conversion is definitely
>>> inconsistent, as it only replaces one part (dots->slashes), not both
>>> (it will convert the example above to "root/some/weird/entry", which
>>> is definitely wrong!). Hence, I suggested using normalize_sysctl(),
>>> which does this properly.
>>>
>>> Obviously, the result is compared against file-system paths. However,
>>> that does not mean the input of --prefix is a file-system path. In
>>> fact, given the current code it is very likely *NOT* a file-system
>>> path. Otherwise, we wouldn't do the conversion.
>>>
>>> The normalize_sysctl() helper uses some hack to allow both input
>>> types: It relies on the first part of the path/name to be
>>> alphanumeric. This is true for all sysctl nodes, therefore, we can
>>> simply look for the first dot or slash and deduce the format. If it's
>>> a slash, it's already a filesystem path, if it's a dot, it needs to be
>>> converted. Thus, using normalize_sysctl() will allow using either
>>> format with --prefix.
>>>
>>> Does that make sense? If yes, I can write a patch and apply it.
>>
>> Yes, I think it does.
>>
>> Perhaps my testing is somewhat limited. This is what I have in
>> /etc/sysctl.conf:
>>
>> net.ipv4.conf.ens3/1.rp_filter = 0
>>
>> In sysctl.c:94, there's "if (path_startswith(p, *i)) ...", where p
>> always starts with "/proc/sys", and *i is the prefix. My point was that
>> whatever you put in the prefix will never match, unless it starts with a
>> slash, therefore is NOT subject to normalize.
>
> I always assumed we add the /proc/sys prefix to --prefix arguments,
> too. But we clearly don't. Sorry, I totally missed that.. You're
> obviously right, it doesn't make sense to specify "--prefix
> /proc/sys/net.ipv6.conf.ens3/1.rp_filter".
>
> I don't think the intention was to require /proc/sys prefixes on the
> command line. This clearly doesn't make much sense. I'd rather expect
> something like this:
>
> char *p;
>
> p = normalize_sysctl(optarg);
> p = strappend("/proc/sys/", p);
> if (!p)
>         return log_oom();
>
> r = strv_consume(&arg_prefixes, p);
> if (r < 0)
>         return log_oom();
>
>
> Your original patch is right, too. But I'm not sure which one to
> prefer. Given that we export systemd-sysctl as rpm macro, I guess we
> have to go with your patch. Otherwise, we'd break ABI.
>
> Thanks and sorry for the confusion!
> David

Not requiring /proc/sys prefixes and normalizing them totally makes
sense. I'm glad we cleared the confusion!

Cheers,
-- 
Jan Synacek
Software Engineer, Red Hat