Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix spaces in output of sysctl -n <key> #42

Closed
wants to merge 2 commits into from
Closed

Fix spaces in output of sysctl -n <key> #42

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 7, 2016

No description provided.

@thias
Copy link
Owner

thias commented Apr 8, 2016

I think I see what you're doing here, but I also think that it fixes some cases and breaks others. See #40 for some details. Basically, we would need to mangle both the value we are trying to set/compare and the value the system reports if we want to avoid breakage.

This is what my module was doing in the past, but it also had its own downsides : The code is more complex and more fragile, and AFAICT some sysctl values could contain multiple spaces on purpose (such as kernel.core_pattern), though in practice it might not be relevant...

For me it makes no doubt that the proper solution is to make sure the values being set are exactly the same as the ones sysctl reports, this means using tabs as-is. This is up to the users of the module. The real problem is that people have been used to setting mangled values, such as grouping kernel.sem with spaces instead of tabs...

Would a clear mention of this in the README be enough, perhaps?

@ghost
Copy link
Author

ghost commented Apr 8, 2016

\s includes [ \t\r\n\f]. So it should not break others.

yes you are right that both value return by the command and the one provided should be parsed or provide the same that sysctl reports. I started to look at parsing the value provided to match the one parsed with sed and i haven't finished.

@ghost
Copy link
Author

ghost commented Apr 8, 2016

i have added parsing of the value provided. Let me know what you think.

@ghost
Copy link
Author

ghost commented Apr 11, 2016

@thias que penses-tu du dernier changement?

@ghost
Copy link
Author

ghost commented Apr 13, 2016

@thias what do you think?

@dantremblay
Copy link

@thias please let us know what you think of this change. We could use this in our environment. Thanks!

@nbentoumi
Copy link

👍

@pckls
Copy link

pckls commented Apr 26, 2016

@juliengk I was about to hack something up to solve this very issue.

One thing I've noticed from testing in a shell is that there is a difference between single vs double quotes for those sed statements - http://stackoverflow.com/questions/6697753/difference-between-single-and-double-quotes-in-bash

When I use /sbin/sysctl -n net.ipv4.tcp_wmem | sed -r 's/\\s+/ /g' the tabs are still there however /sbin/sysctl -n net.ipv4.tcp_wmem | sed -r "s/\\s+/ /g" works, I assume because it's not interpreting the backslash as a literal?

@pckls
Copy link

pckls commented Apr 26, 2016

I've decided to just pass in my Hiera variables with the tabs because I think it's the cleanest solution.

@benohara
Copy link

I see this after an upgrade...this looks to fix it for me

-          unless  => "/usr/bin/test \"$(/sbin/sysctl -n ${qtitle})\" = ${qvalue}",
+          unless  => "/usr/bin/test \"$(/sbin/sysctl -n ${qtitle} | /bin/sed 's/\t/ /g')\" = ${qvalue}",

@bschonec
Copy link

I can confirm that @benohara code change made {my} runs idempotent.

@ghost ghost closed this Jul 27, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants