Nir Soffer has posted comments on this change. Change subject: Using startCmd func for running async execution ......................................................................
Patch Set 2: (6 comments) https://gerrit.ovirt.org/#/c/60237/2/lib/vdsm/commands.py File lib/vdsm/commands.py: Line 65: printable = command Line 66: Line 67: execCmdLogger.debug(cmdutils.command_log_line(printable, cwd=cwd)) Line 68: Line 69: p = CPopen(command, close_fds=True, cwd=cwd, env=env) > redundant and its splitting the code and make it uglier You can do: extra = {} if deathSignal is not None: extra['deathSignal'] = deathSignal CPopen(cmd, ..., **extra) This is much better then duplicating code as you do here. Line 70: Line 71: with terminating(p): Line 72: (out, err) = p.communicate(data) Line 73: Line 70: Line 71: with terminating(p): Line 72: (out, err) = p.communicate(data) Line 73: Line 74: if out is None: > its not related to the patch.. you can post a patch for that separately OK, we can wait with this. Line 75: # Prevent splitlines() from barfing later on Line 76: out = "" Line 77: Line 78: execCmdLogger.debug(cmdutils.retcode_log_line(p.returncode, err=err)) Line 83: Line 84: return p.returncode, out, err Line 85: Line 86: Line 87: def startCmd(command, sudo=False, cwd=None, data=None, raw=False, > I prefer to keep the same arguments in this patch. we can remove redundant I agree with the steps by step, but the steps should be: - Introduce start_cmd doing *only* what we need (nobody calls it) - convert existing code to call new method, separate patches by group. And not duplicating the arguments we cannot and should not support from the old crapy execCmd. Line 88: printable=None, env=None, nice=None, ioclass=None, Line 89: ioclassdata=None, setsid=False, execCmdLogger=logging.root, Line 90: deathSignal=0, resetCpuAffinity=True): Line 91: """ Line 85: Line 86: Line 87: def startCmd(command, sudo=False, cwd=None, data=None, raw=False, Line 88: printable=None, env=None, nice=None, ioclass=None, Line 89: ioclassdata=None, setsid=False, execCmdLogger=logging.root, > same See my comment above Line 90: deathSignal=0, resetCpuAffinity=True): Line 91: """ Line 92: Executes an external command, optionally via sudo. Line 93: Line 111: Line 112: execCmdLogger.debug(cmdutils.command_log_line(printable, cwd=cwd)) Line 113: Line 114: p = AsyncProc(CPopen(command, close_fds=True, cwd=cwd, env=env, Line 115: deathSignal=deathSignal)) > no helper man ... I have limited time to review this, please fix this in this patch. Line 116: if data is not None: Line 117: p.stdin.write(data) Line 118: p.stdin.flush() Line 119: Line 114: p = AsyncProc(CPopen(command, close_fds=True, cwd=cwd, env=env, Line 115: deathSignal=deathSignal)) Line 116: if data is not None: Line 117: p.stdin.write(data) Line 118: p.stdin.flush() > there are tests that use it Delete the useless tests (can be done in separate patch before this patch) Line 119: Line 120: return p Line 121: Line 122: -- To view, visit https://gerrit.ovirt.org/60237 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id38507a04a124918f3865cf011bbf5adc7bc31d5 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@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
