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 <phora...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
