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
--
To view, visit
https://gerrit.ovirt.org/61482
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icbb5a2d3ac439a334db2c9075376f219c356762c
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes