[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 11:26 AM, Jan Synacek <jsynacek at redhat.com> wrote:
>> David Herrmann <dh.herrmann at gmail.com> writes:
>>> Hi
>>>
>>> On Mon, Sep 15, 2014 at 10:00 AM, Jan Synacek <jsynacek at redhat.com> wrote:
>>>> David Herrmann <dh.herrmann at gmail.com> writes:
>>>>> Nevertheless, the documentation should clearly state which input is
>>>>> expected and the current code is definitely wrong as it only performs
>>>>> one way conversions.
>>>>
>>>> Could you please point me to the right documentation that I can update?
>>>> My guess would be sysctl.d(5), since systemd-sysctl(8) refers to it.
>>>> Also, should I make it a separate patch?
>>>
>>> I was referring to the --help text of the binary. There is no man-page
>>> for it, so no need to update any of them.
>>>
>>> Feel free to put me on CC on v2 and I'll apply the patch.
>>>
>>> Thanks a lot!
>>> David
>>
>> After tinkering with it for a while, I think that my original patch is
>> correct. The prefix is a path prefix that is matched against actual
>> paths, not variable names. That means that the prefix always has to
>> start with a slash, otherwise nothing matches, which also means that
>> normalizing would have no effect. And I think that the docstring doesn't
>> need an update either. It says "Only apply rules that apply to *paths
>> with the specified prefix*" (my emphasis added).
>
> 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.

If my example were to start with a slash, than, again, I will have to
begin the prefix with a slash, otherwise it would never match.

The question is, should p always start with "/proc/sys" before it's
matched against the prefix? If yes, than the second conversion is not
needed.

I'm sorry if I still don't fully understand. Maybe a counterexample that
would show where my reasoning fails would be useful.

I'm attaching a v2 patch with the normalization of the prefix, in case
it's really needed. Feel free to modify/rewrite it if it's still needed.

Cheers,
-- 
Jan Synacek
Software Engineer, Red Hat

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-sysctl-correctly-normalize-prefix.patch
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20140915/6c3ca55f/attachment-0001.ksh>