- Notifications
You must be signed in to change notification settings - Fork382
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Allow having a read-only config file#1583
base:master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@## master #1583 +/- ##==========================================- Coverage 37.47% 37.46% -0.01%========================================== Files 127 127 Lines 31023 31026 +3 Branches 93 93 ========================================== Hits 11625 11625- Misses 19349 19352 +3 Partials 49 49
Continue to review full report at Codecov.
|
b540b3b toc6fdf85CompareInstead of locking the config file, open a `<config>.lock` fileand put the fcntl lock on it.Before this change, the config file needed to be opened read-write,even when we only want to read it, because of fcntl locks.The locks are meant to prevent two ZNCs from running on the same config,when they could overwrite each others' config changes.However, since any configs writes and rehashes always reopen the configby path, it's the path to the config that we want to lock,not the file itself(eg. if someone moves znc.conf to a different directory and createsa new one in our directory, we still don't want another znc instanceto run using the new config, but we don't mind another znc instancerunning using the old config in its new location).Therefore, a separate lock file is not worse than locking the config fileitself, and it will allow to open the config read-only.
d8a0e8f to47e0fb7CompareAs suggested by MetaNova on #znc-dev, made it take a |
Sometimes it is desirable for ZNC not to overwrite its config,eg. because the config comes from external configuration management tools.Add a commandline option to set ZNC in read-only mode, in whichit will open the config file read-only, and will refuse to overwrite it.Note that this does not affect .registry files, which can still be written.Also add a big fat warning in the console when running in read-only mode.
I'm not a dev, butI'm going to give this a -1 based on my testing and the discovery ofsome undesired behavior/results coming from my testing these changes. The most prevalent issue is that when you try and issue Also, you have a case where if you do anything that'd result in a config file write, it has the tendency to be able to spam the user with error messages (if they're an admin, I assume, but that's not tested). You can end up with four messages rapidfired within a minute worth of "spam" to your notices tab or to your active screen if your client doesn't split them out into other tabs: If we're running in read only mode, weexpect that writing the config file will fail, or that it won't even attempt to write the config file, and therefore we shouldn't be getting these errors. Further, if we're running in Read Only mode, we shouldn't have a hard failure when we attempt to use My suggestions are: (1) If we're running in Read Only mode, recognize that and don't crit-fail when you call More to come as I do some more tests... |
- don't call WriteConfig() outside of webadmin when in read-only mode- print a more descriptive error message about readonly mode on /znc saveconfig, and on ECONFIG_NEED_VERBOSE_WRITE (i.e. SIGUSR1)- don't print any error messages when in read-only mode and failing/skipping a regular ECONFIG_NEED_WRITE (eg. after channel join)- proceed with /znc shutdown without writing config when in read-only mode
@teward thanks for spotting this, I really should have tested my change better. I'd love if some dev looked that latest commit and told me if there's a better way to do it. |
| @@ -229,6 +229,15 @@ void CZNC::Loop() { | |||
| // stop pending configuration timer | |||
| DisableConfigTimer(); | |||
| if (GetReadonlyConfig()) { | |||
| if (eState == ECONFIG_NEED_VERBOSE_WRITE) { | |||
| Broadcast("Writing the config skipped because " | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This will regularly spam admins whenever e.g. "in-config" channel's key is changing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thought these changes trigger non-verbose config saves... I grepped forECONFIG_NEED_VERBOSE_WRITE and the only place that sets it that I found wasSIGUSR1 handler.
| m_bReadonlyConfig = bReadonlyConfig; | ||
| if (m_bReadonlyConfig) { | ||
| CUtils::PrintError("Running in read-only mode. Any changes to the settings, users,"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is it an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wanted it to be a warning, but couldn't find anything likePrintWarning. I looked at what--allow-root does, and it prints its warning as an error.
Does this mode also prevent writing of .registry files? |
Many module callbacks return |
No. I thought it'd be better if it doesn't affect the .registry files.
So in this case it'd be |
Then some random changes will be saved, and some other changes will be not.
That's not what I meant... You wanted an example of enum return value instead of bool? |
Now that I think of it... yeah, that'll need to be read-only too.
I didn't want to introduce a new enum that'll be used in only one place. But maybe I should... |
This should technicallyfix#975 for the narrow usecase described by that issue's OP.
Later I'm going to submit another pull request, taking care of#1253, which should take care of most usecases where part of the config is supposed to be provided externally, while the rest of it should be configurable through znc (eg. the users should still be allowed to edit their own settings, add networks, etc.)