Nir Soffer has posted comments on this change. Change subject: mkimage: lib: add and use umaskset helper ......................................................................
Patch Set 3: Code-Review-1 (7 comments) https://gerrit.ovirt.org/#/c/48133/3/tests/cmdutilsTests.py File tests/cmdutilsTests.py: Line 104: class UmasksetTests(VdsmTestCase): Line 105: Line 106: def setUp(self): Line 107: testdir = os.path.dirname(os.path.abspath(__file__)) Line 108: umaskset = os.path.join(testdir, '..', 'vdsm', 'umaskset') We can use CommandPath to find the tool in the module level, and monkeypatch the test itself. can be much shorter and more robust. Using Patch in setUp() and tearDown() is fragile and should be used only if we don't have another choice. Line 109: Line 110: self.patch = Patch([ Line 111: (constants, "EXT_UMASKSET", umaskset), Line 112: ]) Line 127: Line 128: self.assertEqual(umask, _umask_from_output(out)) Line 129: Line 130: Line 131: def _umask_from_output(output): Can you show the output from the shell your are parsing here? Also, separating the split and the int conversion will be easier to read. https://gerrit.ovirt.org/#/c/48133/3/tests/mkimageTests.py File tests/mkimageTests.py: Line 74: self.expected_results[longpath] = content Line 75: self.files[longpath] = b64encode(content) Line 76: Line 77: self.patch = Patch([ Line 78: (cmdutils, "umaskset", _umaskset), We simplify using lambda: (cmdutils, "umaskset", lambda cmd, mask: cmd), But why do we need to monkeypatch this in the test? Don't we want to run the test with umaskset? Line 79: (mkimage, "DISKIMAGE_USER", -1), Line 80: (mkimage, "DISKIMAGE_GROUP", -1), Line 81: (mkimage, "_P_PAYLOAD_IMAGES", self.img_dir), Line 82: ]) https://gerrit.ovirt.org/#/c/48133/3/vdsm.spec.in File vdsm.spec.in: Line 883: %{_libexecdir}/%{vdsm_name}/fc-scan Line 884: %{_libexecdir}/%{vdsm_name}/persist-vdsm-hooks Line 885: %{_libexecdir}/%{vdsm_name}/unpersist-vdsm-hook Line 886: %{_libexecdir}/%{vdsm_name}/ovirt_functions.sh Line 887: %{_libexecdir}/%{vdsm_name}/umaskset This name match taskset, but not the other tools we keep in libexec/vdsm. How about calling it "umask" - similar to "nice" and "ionice". tasket is a bit odd compared to other unix tools names. Line 888: %{_libexecdir}/%{vdsm_name}/vdsm-gencerts.sh Line 889: %{_libexecdir}/%{vdsm_name}/vdsmd_init_common.sh Line 890: %{_datadir}/%{vdsm_name}/network/__init__.py* Line 891: %{_datadir}/%{vdsm_name}/network/api.py* https://gerrit.ovirt.org/#/c/48133/3/vdsm/mkimage.py File vdsm/mkimage.py: Line 126: isopath = _getFileName(vmId, files) Line 127: Line 128: command = [ Line 129: EXT_MKISOFS, '-R', '-J', '-o', isopath Line 130: ] This indentation does not improve anything. Can we avoid it for short lists? Line 131: if volumeName is not None: Line 132: command.extend(['-V', volumeName]) Line 133: command.extend([dirname]) Line 134: Line 131: if volumeName is not None: Line 132: command.extend(['-V', volumeName]) Line 133: command.extend([dirname]) Line 134: Line 135: cmd = cmdutils.umaskset(command, _UMASK) Can you explain why we need this? Is it possible that we can solve this problem without modifying umask? Line 136: Line 137: rc, out, err = execCmd(cmd, raw=True) Line 138: if rc: Line 139: raise OSError(errno.EIO, "could not create iso file: " https://gerrit.ovirt.org/#/c/48133/3/vdsm/umaskset File vdsm/umaskset: Line 1: #!/bin/sh Line 2: umask $1 Line 3: shift Line 4: exec $@ How about writing this in C - will not be much longer, and adding this tool to coreutils? -- To view, visit https://gerrit.ovirt.org/48133 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cecb972405a385af8b19542780682eeb394fd12 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
