fix: lock config file when reading and writing #579
+68
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/backport to stable30 |
|
/backport to stable29 |
|
/backport to stable28 |
this is in accordance how the config files are handled in \OC\Config Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
Signed-off-by: Arthur Schiwon <[email protected]>
blizzz
force-pushed
the
fix/lock-config
branch
from
42e81d8 to
e2f238d
Compare
| @@ -89,9 +89,23 @@ public function __construct(string $baseDir) { | |||
| if (!file_exists($configFileName)) { | |||
| throw new \Exception('Could not find config.php. Is this file in the "updater" subfolder of Nextcloud?'); | |||
| } | |||
| $filePointer = @fopen($configFileName, 'r'); | |||
| if ($filePointer === false) { | |||
| throw new \Exception('Could not open config.php.'); | |||
There was a problem hiding this comment.
Suggested change
| throw new \Exception('Could not open config.php.'); | |
| throw new \Exception('Could not open config.php ' . $configFileName); |
There was a problem hiding this comment.
I'm a fan of including the $configFileName in the output (there are other spots too) since:
- full paths being on-hand helps make troubleshooting easier
- there may eventually be support for multiple config files at the Updater level (like there is already at Server)
| @@ -89,9 +89,23 @@ public function __construct(string $baseDir) { | |||
| if (!file_exists($configFileName)) { | |||
| throw new \Exception('Could not find config.php. Is this file in the "updater" subfolder of Nextcloud?'); | |||
There was a problem hiding this comment.
Suggested change
| throw new \Exception('Could not find config.php. Is this file in the "updater" subfolder of Nextcloud?'); | |
| throw new \Exception('Could not find config.php ' . $configFileName); |
| throw new \Exception('Could not open config.php.'); | ||
| } | ||
| if (!flock($filePointer, LOCK_SH)) { | ||
| throw new \Exception(sprintf('Could not acquire a shared lock on the config file')); |
There was a problem hiding this comment.
Suggested change
| throw new \Exception(sprintf('Could not acquire a shared lock on the config file')); | |
| throw new \Exception(sprintf('Could not acquire a shared lock on the config file %s', $configFileName)); |
nit: Generating the exceptions consistently would be preferred (i.e. all with sprintf or all without, but not both).
| } | ||
|
|
||
| try { | ||
| require_once $configFileName; |
There was a problem hiding this comment.
nit: here we're using require_once; below we're using require (and it looks like in server we use include in the the Config class.
This was referenced Jan 14, 2025
This was referenced Jan 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is in accordance how the config files are handled in \OC\Config
I successfully upgraded from 29.0.4 to 29.0.5 with updater.phar.