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

nsoffer at redhat.com nsoffer at redhat.com
Fri Mar 28 01:42:58 UTC 2014


Nir Soffer has posted comments on this change.

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


Patch Set 4: Code-Review-1

(5 comments)

The implementation is too complex and can be simplified to one line change.

There are few unrelated changes - import cleanup and whitespace that should be submitted separately.

I like the tests - but lets split them to another patch, and rebase the function changes on the tests patch. This will make it harder to break the function and easier to review.

The test can be simplified and using MonkeyClass decorator, and the function names can be cleaner.

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

Line 53: import threading
Line 54: import time
Line 55: import zombiereaper
Line 56: 
Line 57: import cpopen
> I do not mind this change, but it's unrelated to this patch.
I really like this change, in its own patch.
Line 58: from .config import config
Line 59: from . import constants
Line 60: 
Line 61: # Buffsize is 1K because I tested it on some use cases and 1K was fastest. If


Line 652:     IMPORTANT NOTE: the new process would receive `deathSignal` when the
Line 653:     controlling thread dies, which may not be what you intended: if you create
Line 654:     a temporary thread, spawn a sync=False sub-process, and have the thread
Line 655:     finish, the new subprocess would die immediately.
Line 656:     """
Too complicated - to support tuples (and iteratables) we can simply do:

    command = list(command)

And we don't have to change the rest of the code.
Line 657:     if ioclass is not None:
Line 658:         cmd = command
Line 659:         command = [constants.EXT_IONICE, '-c', str(ioclass)]
Line 660:         if ioclassdata is not None:


Line 687:     p = AsyncProc(p)
Line 688:     if not sync:
Line 689:         if data is not None:
Line 690:             p.stdin.write(data)
Line 691:             p.stdin.flush()
This whitespace change is not related.
Line 692:         return p
Line 693: 
Line 694:     (out, err) = p.communicate(data)
Line 695: 


http://gerrit.ovirt.org/#/c/26070/4/tests/utilsTests.py
File tests/utilsTests.py:

Line 476:     def setUp(self):
Line 477:         self.patch.apply()
Line 478: 
Line 479:     def tearDown(self):
Line 480:         self.patch.revert()
You don't need to implement setUp and tearDown - use MonkeyClass instead.

 144 #
 145 # @MonkeyClass(subprocess, 'Popen', lambda x: None)
 146 # @MonkeyClass(os, 'chown', lambda *x: 0)
 147 # class TestSomething():
 148 #
 149 #     def testThis(self):
 150 #         # using patched functions
 151 #
 152 #     def testThat(self):
 153 #         # using patched functions
Line 481: 
Line 482:     def _execCmdReturnComm(self, *args, **kwargs):
Line 483:         proc = utils.execCmd(*args, **kwargs)
Line 484:         proc.wait()


Line 510:         expected = (constants.EXT_SUDO,
Line 511:                     utils.SUDO_NON_INTERACTIVE_FLAG) + self.args
Line 512:         command = self._execCmdReturnComm(func(self.args), sync=False,
Line 513:                                           sudo=True)
Line 514:         self.assertEquals(expected, command)
It whould be more clear and clean if you name your checking methods checkXXX and your tests testXXX instead of the _test:

    def testNormalList(self):
        self.checkNormal(list)

    def checkNormal(self, what):
        pass
Line 515: 
Line 516:     def testNormalList(self):
Line 517:         self._testNormal(list)
Line 518: 


-- 
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: 4
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: Nir Soffer <nsoffer 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