Yaniv Bronhaim has posted comments on this change.
Change subject: utils: atomic write ......................................................................
Patch Set 8: Code-Review-1
(5 comments)
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_write(path, flag): call it - atomic_file_write_context. and if its only write, remove the flag param Line 937: """Atomically write into a file. Line 938: Line 939: Usage: Line 940:
Line 942: f.write('shrubery') Line 943: # there are no changes on foo.txt yet Line 944: # now it is changed Line 945: """ Line 946: tmp_path = _generate_tmp_file_path(path) path is a file name and not a path, at least if I follow the usage comment Line 947: if os.path.exists(path): Line 948: shutil.copyfile(path, tmp_path) Line 949: try: Line 950: with open(tmp_path, flag) as f:
Line 946: tmp_path = _generate_tmp_file_path(path) Line 947: if os.path.exists(path): Line 948: shutil.copyfile(path, tmp_path) Line 949: try: Line 950: with open(tmp_path, flag) as f: you should use the full path I guess, or call it filename. its confusing Line 951: yield f Line 952: except: Line 953: raise Line 954: else:
Line 953: raise Line 954: else: Line 955: shutil.copyfile(tmp_path, path) Line 956: finally: Line 957: os.remove(tmp_path) use rmFile, or handle exceptions Line 958: Line 959: Line 960: def _generate_tmp_file_path(file_path):
Line 957: os.remove(tmp_path) Line 958: Line 959: Line 960: def _generate_tmp_file_path(file_path): Line 961: return '{}.{}.tmp'.format(file_path, random_name()) you don't need this helper, it got no use - do it directly in the function