Petr Horáček has posted comments on this change.
Change subject: utils: atomic file write ......................................................................
Patch Set 11: Verified+1
(12 comments)
Passed utilsTests.py
https://gerrit.ovirt.org/#/c/61482/8/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 932: return rget(dict.get(keys[0]), keys[1:], default) Line 933: Line 934: Line 935: @contextmanager Line 936: def atomic_file_write(filename, flag):
call it - atomic_file_write_context. and if its only write, remove the flag
Write methods are available in 'w', 'a' and '+' flags. We should keep the flag argument. Line 937: """Atomically write into a file. Line 938: Line 939: Usage: Line 940:
Line 932: return rget(dict.get(keys[0]), keys[1:], default) Line 933: Line 934: Line 935: @contextmanager Line 936: def atomic_file_write(filename, flag):
As I see it, it's like asking to specify the type name in the variable (or
I think it is obvious from its usage. Plus none of our context manager functions has _context in it. Line 937: """Atomically write into a file. Line 938: Line 939: Usage: Line 940:
Line 942: f.write('shrubbery') Line 943: # there are no changes on foo.txt yet Line 944: # now it is changed Line 945: """ Line 946: fd, tmp_filename = tempfile.mkstemp(
path is a file name and not a path, at least if I follow the usage comment
Done Line 947: dir=os.path.dirname(os.path.abspath(filename)), Line 948: prefix=os.path.basename(filename) + '.', Line 949: suffix='.tmp') Line 950: os.close(fd)
Line 946: fd, tmp_filename = tempfile.mkstemp( Line 947: dir=os.path.dirname(os.path.abspath(filename)), Line 948: prefix=os.path.basename(filename) + '.', Line 949: suffix='.tmp') Line 950: os.close(fd)
you should use the full path I guess, or call it filename. its confusing
I renamed it to 'file'. Python 3 uses the same for open() https://docs.python.org/3.5/library/functions.html#open Line 951: try: Line 952: if os.path.exists(filename): Line 953: shutil.copyfile(filename, tmp_filename) Line 954: with open(tmp_filename, flag) as f:
Line 953: shutil.copyfile(filename, tmp_filename) Line 954: with open(tmp_filename, flag) as f: Line 955: yield f Line 956: except: Line 957: rmFile(tmp_filename)
use rmFile, or handle exceptions
Done Line 958: raise Line 959: else:
Line 956: except: Line 957: rmFile(tmp_filename) Line 958: raise Line 959: else: Line 960: os.rename(tmp_filename, filename)
tempfile.NamedTemporaryFile(dir=os.path.dirname(file_path))
Done
Line 957: rmFile(tmp_filename) Line 958: raise Line 959: else: Line 960: os.rename(tmp_filename, filename) Line 961
you don't need this helper, it got no use - do it directly in the function
Done
https://gerrit.ovirt.org/#/c/61482/9/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 932: return rget(dict.get(keys[0]), keys[1:], default) Line 933: Line 934: Line 935: @contextmanager Line 936: def atomic_file_write(filename, flag):
file is a saved word.. better to use file_name.
In the following patch I use 'r+' to read and edit file inside one with block.
Changed to filename. Line 937: """Atomically write into a file. Line 938: Line 939: Usage: Line 940:
Line 933: Line 934: Line 935: @contextmanager Line 936: def atomic_file_write(filename, flag): Line 937: """Atomically write into a file.
I don't know why in rget above the doc string is in that format, but please
I'm following https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings Line 938: Line 939: Usage: Line 940: Line 941: with atomic_write('foo.txt', 'w') as f:
PS9, Line 942: b
bb
Thanks!
Line 942: f.write('shrubbery') Line 943: # there are no changes on foo.txt yet Line 944: # now it is changed Line 945: """ Line 946: fd, tmp_filename = tempfile.mkstemp(
you can't ignore the fd. you need to close it
Done Line 947: dir=os.path.dirname(os.path.abspath(filename)), Line 948: prefix=os.path.basename(filename) + '.', Line 949: suffix='.tmp') Line 950: os.close(fd)
PS9, Line 957: tmp_file
copyfile is NOT atomic. you must use os.rename() which is promised by POSIX
Done