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

[David Herrmann]

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