Change in vdsm[master]: util: fix execCmd to accept tuples

ybronhei at redhat.com ybronhei at redhat.com
Thu Mar 27 11:50:06 UTC 2014


Yaniv Bronhaim has posted comments on this change.

Change subject: util: fix execCmd to accept tuples
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.ovirt.org/#/c/26070/2/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 674:             command = itertools.chain([constants.EXT_SUDO,
Line 675:                                        SUDO_NON_INTERACTIVE_FLAG],
Line 676:                                       command)
Line 677: 
Line 678:     command = list(command)
> why is this a list() and not tuple()? Does  CPopen require that? (it should
it can be tuple cast instead, cpopen knows to work with tuple. dima just wanted to make it not iterable iiuc
Line 679:     if not printable:
Line 680:         printable = command
Line 681: 
Line 682:     cmdline = repr(subprocess.list2cmdline(printable))


Line 690:             p.stdin.write(data)
Line 691:             p.stdin.flush()
Line 692:         if attachCommand:
Line 693:             # vdsm prefix to avoid stepping on existing members
Line 694:             setattr(p, 'vdsm_command', command)
I'd make this attribute mandatory on AyncProc object, and not only for testing
Line 695:         return p
Line 696: 
Line 697:     (out, err) = p.communicate(data)
Line 698: 


-- 
To view, visit http://gerrit.ovirt.org/26070
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8d1269d9a26b1fef1552976ae626cf4596471283
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list