1719725: rhsm - Write config file atomically#2120
1719725: rhsm - Write config file atomically#2120jirihnidek merged 1 commit intocandlepin:masterfrom
Conversation
|
Can one of the admins verify this patch? |
|
@candlepin-bot ok to test |
1 similar comment
|
@candlepin-bot ok to test |
jirihnidek
left a comment
There was a problem hiding this comment.
Hi, thanks for PR. I have small proposal how to improve it.
src/rhsm/config.py
Outdated
| """Writes config file to storage.""" | ||
| fo = open(self.config_file, "w") | ||
| self.write(fo) | ||
| with open(self.config_file + ".tmp", "w") as fo: |
There was a problem hiding this comment.
This is better than previous solution, but I would like to improve it a little bit more. Why? Let's consider following situation. Two processes try to write to the /etc/rhsm/rhsm.conf.tmp in the same time. The configuration file can be corrupted, because self.write(fo) can be interrupted. I propose something like this:
import tempfile
import os
dir_name = os.path.dirname(self.config_file)
temp_filename = tempfile.mktemp(prefix='rhsm.', suffix='.tmp', dir=dir_name)
with open(temp_filename, "w") as fo:
self.write(fo)
os.rename(self.config_file + ".tmp", self.config_file)As you can see each process creates unique temporary file and renaming should be atomic.
There was a problem hiding this comment.
Yes, I was thinking the same.
mktemp is deprecated, though. I'll use NamedTemporaryFile.
There was a problem hiding this comment.
mktempis deprecated, though. I'll useNamedTemporaryFile.
Sure, the code was just suggestion how to solve the issue. :-)
880da3a to
f74c120
Compare
jirihnidek
left a comment
There was a problem hiding this comment.
Hi, thanks for updating your PR. 👍 I have we two more requests. 😃
src/rhsm/config.py
Outdated
| def save(self, config_file=None): | ||
| """Writes config file to storage.""" | ||
| fo = open(self.config_file, "w") | ||
| fo = tempfile.NamedTemporaryFile(mode="w", dir="/etc/rhsm", delete=False) |
There was a problem hiding this comment.
- Please use
dir=os.path.dirname(self.config_file). Your implementation breaks several unit tests and the configuration directory can be different than '/etc/rhsm' (e.g. containers). - Please use
try: except:here, becauserhsmis module that can be used by other application. When the application is not executed by root, then it will raiseOSErrorexception.
There was a problem hiding this comment.
Please use dir=os.path.dirname(self.config_file).
Ouch, but of course!
There was a problem hiding this comment.
Please use try: except: here, because rhsm is module that can be used by other application. When the application is not executed by root, then it will raise OSError exception.
What should be done in the except: branch? The current code on master does not use try/except, so maybe we fix that in a separate PR?
There was a problem hiding this comment.
Please use dir=os.path.dirname(self.config_file).
Ouch, but of course!
Done.
There was a problem hiding this comment.
Please use try: except: here, because rhsm is module that can be used by other application. When the application is not executed by root, then it will raise OSError exception.
What should be done in the
except:branch? The current code on master does not use try/except, so maybe we fix that in a separate PR?
Good point. rhsm is module and it is not possible to e.g. log anything to rhsm.log. Let's do not use try-except and application using this module will have to handle exceptions.
src/rhsm/config.py
Outdated
| fo = open(self.config_file, "w") | ||
| fo = tempfile.NamedTemporaryFile(mode="w", dir="/etc/rhsm", delete=False) | ||
| self.write(fo) | ||
| fo.close() |
There was a problem hiding this comment.
I prefer to use with statement. When some exception happens inside with statement, then the file is closed automatically.
f74c120 to
601fa44
Compare
The config file is read asynchronously by at least the file watches of dbus/server.py, and if it is just overwritten in place the file watch handler might read it when it has only been partially written.
601fa44 to
195c154
Compare
The config file is read asynchronously by at least the file watches of
dbus/server.py, and if it is just overwritten in place the file watch
handler might read it when it has only been partially written.