Alon Bar-Lev has posted comments on this change. Change subject: core: Add executePipe ......................................................................
Patch Set 3: (11 comments) some minor issues. please confirm that rhel-6.5 pep8 is ok with this code as well. http://gerrit.ovirt.org/#/c/26213/3/src/otopi/plugin.py File src/otopi/plugin.py: Line 393: waited = 0 Line 394: while not finished: Line 395: finished = None not in [ Line 396: p.poll() for p in popens Line 397: ] or max_delays is not None and waited > max_delays please each conditional within own line finished = ( None not in [ ] or max is not ... and waited > ... ) however, in these complex condition please add () to make it clear finished = ( None not in [ ] or ( max is not ... and waited > ... ) ) Line 398: if not finished: Line 399: time.sleep(delay) Line 400: if waited % 30 == 1: Line 401: self.logger.debug( Line 410: popenKwargsList, Line 411: exc_info=True Line 412: ) Line 413: raise Line 414: please print execution result for each command Line 415: return { Line 416: 'stdout': popens[-1].stdout, Line 417: 'result': [ Line 418: { Line 459: """ Line 460: if logStreams and stdin is not None: Line 461: self.logger.debug( Line 462: 'executePipe-input: %s stdin:\n%s\n', Line 463: ' | '.join([str(kw['args']) for kw in popenKwargsList]), not sure we need all commands but the first, we do get all commands at the raw function. Line 464: '\n'.join(stdin) Line 465: ) Line 466: Line 467: if stdin is not None: Line 465: ) Line 466: Line 467: if stdin is not None: Line 468: temp_stdin, temp_name = tempfile.mkstemp() Line 469: atexit.register(os.unlink, temp_name) why? you can remove it at finally, this file is not used outside of the scope of this function. Line 470: with os.fdopen(temp_stdin, 'w') as f: Line 471: f.write('\n'.join(stdin).encode('utf-8')) Line 472: temp_stdin = open(temp_name, 'r') Line 473: Line 467: if stdin is not None: Line 468: temp_stdin, temp_name = tempfile.mkstemp() Line 469: atexit.register(os.unlink, temp_name) Line 470: with os.fdopen(temp_stdin, 'w') as f: Line 471: f.write('\n'.join(stdin).encode('utf-8')) if stdin is non empty also append '\n' at last char Line 472: temp_stdin = open(temp_name, 'r') Line 473: Line 474: res_dict = self.executePipeRaw( Line 475: popenKwargsList=popenKwargsList, Line 468: temp_stdin, temp_name = tempfile.mkstemp() Line 469: atexit.register(os.unlink, temp_name) Line 470: with os.fdopen(temp_stdin, 'w') as f: Line 471: f.write('\n'.join(stdin).encode('utf-8')) Line 472: temp_stdin = open(temp_name, 'r') can't we reopen the fd? Line 473: Line 474: res_dict = self.executePipeRaw( Line 475: popenKwargsList=popenKwargsList, Line 476: stdin=temp_stdin if stdin is not None else None, Line 496: } Line 497: if logStreams: Line 498: self.logger.debug( Line 499: 'executePipe-output: %s stdout:\n%s\n', Line 500: ' | '.join([str(kw['args']) for kw in popenKwargsList]), not sure we need all commands but the first, we do get all commands at the raw function. Line 501: '\n'.join(res['stdout']) Line 502: ) Line 503: for i, r, kw in [ Line 504: (i, r, popenKwargsList[i]) Line 509: i, Line 510: kw['args'], Line 511: '\n'.join(r['stderr']) Line 512: ) Line 513: RCs = [ lower case please Line 514: (i, r['rc']) Line 515: for i, r in enumerate(res['result']) Line 516: if r['rc'] != 0 Line 517: ] Line 514: (i, r['rc']) Line 515: for i, r in enumerate(res['result']) Line 516: if r['rc'] != 0 Line 517: ] Line 518: if len(RCs) > 0 and raiseOnError: set(r['rc']) != set([0]) ? Line 519: # Log the last one, somewhat similarly to pipefail in bash Line 520: raise RuntimeError( Line 521: _("Command '{command}' failed to execute").format( Line 522: command=popenKwargsList[RCs[-1][0]]['args'][0] Line 518: if len(RCs) > 0 and raiseOnError: Line 519: # Log the last one, somewhat similarly to pipefail in bash Line 520: raise RuntimeError( Line 521: _("Command '{command}' failed to execute").format( Line 522: command=popenKwargsList[RCs[-1][0]]['args'][0] I think that it is better to show the entire command and not the rc? shell like message to user? prog1 | prog2 | prog3 failed Line 523: ) Line 524: ) Line 525: return res Line 526: Line 545: Line 546: Returns: Line 547: (rc, stdout, stderr) Line 548: Line 549: stdout, stderr binary blobs. separate patch please? Line 550: """ Line 551: try: Line 552: if envAppend is not None: Line 553: if env is None: -- To view, visit http://gerrit.ovirt.org/26213 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a434fbe1308ac2f603b8ae09756354c11138912 Gerrit-PatchSet: 3 Gerrit-Project: otopi Gerrit-Branch: master Gerrit-Owner: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
