Nir Soffer has posted comments on this change. Change subject: mkimage: setup right permissions before mkisofs ......................................................................
Patch Set 3: Code-Review-1 (2 comments) https://gerrit.ovirt.org/#/c/48538/3/vdsm/mkimage.py File vdsm/mkimage.py: Line 131: # pre-create the destination iso path with the right permissions; Line 132: # mkisofs/genisoimage will truncate the content and keep the Line 133: # permissions. Line 134: fd = os.open(isopath, os.O_CREAT | os.O_RDONLY | os.O_EXCL, 0o640) Line 135: os.close(fd) This will fail if the file already exists. Possible flow: 1. Create file 2. Run mkisofs 3. vdsm is killed (e.g, panic in another thread) 4. iso file not deleted 5. All further attempt will fail (until reboot? path is in /tmp?) So even if we don't expect the file to exists in normal condition, we should remove before trying. Because we don't expect it to exists, it would be useful to log a warning if found. Line 136: Line 137: rc, out, err = execCmd(command, raw=True) Line 138: if rc: Line 139: # we want to use O_CREAT | O_EXCL, so we need to cleanup Line 135: os.close(fd) Line 136: Line 137: rc, out, err = execCmd(command, raw=True) Line 138: if rc: Line 139: # we want to use O_CREAT | O_EXCL, so we need to cleanup No, this is not good enough to ensure that create will succeed. We clean here because the operation failed and there is not reason to leave the file around. Line 140: removeFs(isopath) Line 141: # skip _commonCleanFs step for missing iso Line 142: isopath = None Line 143: -- To view, visit https://gerrit.ovirt.org/48538 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibc971a39ae2a8eaf7af934bb0922647e4676d03f Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
