David Herrmann <dh.herrm...@gmail.com> writes:
> Hi
>
> On Mon, Sep 15, 2014 at 11:26 AM, Jan Synacek <jsyna...@redhat.com> wrote:
>> David Herrmann <dh.herrm...@gmail.com> writes:
>>> Hi
>>>
>>> On Mon, Sep 15, 2014 at 10:00 AM, Jan Synacek <jsyna...@redhat.com> wrote:
>>>> David Herrmann <dh.herrm...@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
>From 8159821c1f253fde4132c08eb8a39507ce38fd03 Mon Sep 17 00:00:00 2001
From: Jan Synacek <jsyna...@redhat.com>
Date: Fri, 12 Sep 2014 11:10:44 +0200
Subject: [PATCH v2] sysctl: correctly normalize prefix
---
src/sysctl/sysctl.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/sysctl/sysctl.c b/src/sysctl/sysctl.c
index 8ce9870..4454c8f 100644
--- a/src/sysctl/sysctl.c
+++ b/src/sysctl/sysctl.c
@@ -256,12 +256,7 @@ static int parse_argv(int argc, char *argv[]) {
return 0;
case ARG_PREFIX: {
- char *p;
-
- for (p = optarg; *p; p++)
- if (*p == '.')
- *p = '/';
-
+ optarg = normalize_sysctl(optarg);
if (strv_extend(&arg_prefixes, optarg) < 0)
return log_oom();
--
1.9.3
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
