search

Sepero / temp-throttle

A shell script for throttling system CPU frequency based on a desired maximum temperature
Other
243 stars 64 forks source link

Feature configfile #12

Closed martin-schmidt closed 8 years ago

martin-schmidt commented 9 years ago

Config file can be used to set the temperature. File can be named 

/etc/temp_throttle.conf

or

~/.temp_throttle.conf

Config file settings can be overridden with the usual command line argument. Settings from the users home directory override the settings from /etc.

The file must contain the line 'MAX_TEMP=whatever-temperature-you-like' - e.g.:

MAX_TEMP=80

resolve #10

Sepero commented 9 years ago

Hello Martin. I've been thinking a lot about your feature requests and the pull requests you have made. I regret to say that I would rather not merge this pull request at this time. Currently I do not see a high demand or necessity for using a configuration file, and I wish to keep the code as simple as possible. It pains me a bit to tell you this, because I know that you have put quite some time into developing this feature. Just because I am not accepting the pull request now, does not mean I won't add it in the future if demand increases. I want you to know that I really appreciate the work you have done. I think you have done well to compliment the code, and it was professionally done. If it's okay with you, I would like to leave this pull request open for now. If someone asks me about using a configuration file, I will be happy to direct them to your branch/fork. If you have any concerns about this, I am glad to answer. Thank you again.

martin-schmidt commented 9 years ago

Hello Sepero,

I use the file, because I want to be able start the script without options. This makes it easier for me to automate installation on several machines and to start the script as daemon with init. I created a branch with the init script and some debian specific changes here: https://github.com/martin-schmidt/temp-throttle/tree/feature-initscript

Sepero commented 9 years ago

Hello Martin. I have reconsidered your contribution here and I am happy to tell you that I have decided to accept your pull request. It may take me some weeks to implement the code and see if it might be preferable to integrate it a little differently. Thank you again for your contribution! :)

Sepero commented 9 years ago

I've been looking over your code more recently, and it seems you have a lot more to add than I originally understood (init.d startup file etc). I really like almost everything. My only main issue is that you have included the config file by sourcing it. While this is a very easy solution, it is also has a good potential to cause errors, and in worst case scenario it could be used maliciously.

I've spent a good portion of the day thinking of a simple way to parse the config file. I searched online for bash ini parsers, but I'm not very happy with those solutions. Currently I'm thinking I will be going with a grep/sed combination for parsing the config file.

Also, I'm not very knowledgeable about systemd and if your init.d script will work well with it. This is something that will require me doing more research. I'm open to any feedback you may have regarding these things. Thanks again.

martin-schmidt commented 9 years ago

Hello Sepero,

it's necessary to run temp-throttle as uid 0. There is, as far as I know, no reason for a malicious user to escalate privileges if he already p0wnd his way up to root. The only way I can think to exploit this would be a world writeable config file. That wouldn't be a temp-throttle related issue but a misconfiguration. I looked out for other ways to define the variables from a file when I wrote that part. But after starting to write lines to validate the config file, I thought that this wouldn't be worth the effort and source would do the job well enough.

I use the init script with systemd v226 on debian/kali and it works like a charm. The only thing I had to change to make this run after I switched to systemd was fixing this line: https://github.com/martin-schmidt/temp-throttle/commit/4b695e7c807ddb804982fc1c042e4e4d119f9e33.

Sepero commented 9 years ago

Glad to hear back from you Martin. I give you credit for thinking to simply source the file. I actually like the idea a lot. Straight, easy, and to the point. If I wasn't producing temp-throttle for mass consumption, then I certainly would be following suit. Because of the mass consumption, I figured it might be nice for me to make it a little more safe from errors. So, I wrote to let you know my intentions and inquire if you might have any better ideas. Don't worry about the parsing implementation. I will take the time to code it myself. Thanks for getting back with me, it's always great to hear from you.