)]}'
{"neutron/agent/linux/async_process.py":[{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":115,"context_line":"        # Halt the greenthreads"},{"line_number":116,"context_line":"        self._kill_event.send()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"        pid \u003d utils.get_child_pid(self._process.pid, self.root_helper)"},{"line_number":119,"context_line":"        if pid:"},{"line_number":120,"context_line":"            self._kill_process(pid)"},{"line_number":121,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_bd69bdfa","line":118,"updated":"2014-10-27 17:17:20.000000000","message":"+1 to ajo\u0027s suggestion of targeting get_pid_to_kill at this new call.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":115,"context_line":"        # Halt the greenthreads"},{"line_number":116,"context_line":"        self._kill_event.send()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"        pid \u003d utils.get_child_pid(self._process.pid, self.root_helper)"},{"line_number":119,"context_line":"        if pid:"},{"line_number":120,"context_line":"            self._kill_process(pid)"},{"line_number":121,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_eec7a910","line":118,"in_reply_to":"9aa7fdbe_bd69bdfa","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"fd0df208fafeb8a159caaf3836fd2aff8d13867e","unresolved":false,"context_lines":[{"line_number":115,"context_line":"        # Halt the greenthreads"},{"line_number":116,"context_line":"        self._kill_event.send()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"        pid \u003d utils.get_roothelper_child_pid("},{"line_number":119,"context_line":"            self._process.pid, self.root_helper)"},{"line_number":120,"context_line":"        if pid:"},{"line_number":121,"context_line":"            self._kill_process(pid)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_7285c905","line":118,"updated":"2014-11-17 12:33:14.000000000","message":"nit: s/roothelper/root_helper/g.\n\nSorry, it really bothers my OCD senses :(","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"b77bec89c9b59d1de62e1f253b662ee0e263612d","unresolved":false,"context_lines":[{"line_number":115,"context_line":"        # Halt the greenthreads"},{"line_number":116,"context_line":"        self._kill_event.send()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"        pid \u003d utils.get_roothelper_child_pid("},{"line_number":119,"context_line":"            self._process.pid, self.root_helper)"},{"line_number":120,"context_line":"        if pid:"},{"line_number":121,"context_line":"            self._kill_process(pid)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_90fe5104","line":118,"in_reply_to":"5a890539_7285c905","updated":"2014-11-19 16:23:58.000000000","message":"Done","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"}],"neutron/agent/linux/external_process.py":[{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"ebbc92030db6c3008ba2eb6cc86c464eb040e050","unresolved":false,"context_lines":[{"line_number":13,"context_line":"#    under the License."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"import collections"},{"line_number":16,"context_line":"import select"},{"line_number":17,"context_line":"import shlex"},{"line_number":18,"context_line":"import subprocess"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"da9df570_ce713279","line":16,"updated":"2014-09-21 00:18:49.000000000","message":"not used","commit_id":"c8d20a37ec4e7bbfef05c45d62233a1cb90709f0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"25cda7a095e0abb7af2a528f953e25ed73d96762","unresolved":false,"context_lines":[{"line_number":13,"context_line":"#    under the License."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"import collections"},{"line_number":16,"context_line":"import select"},{"line_number":17,"context_line":"import shlex"},{"line_number":18,"context_line":"import subprocess"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"da9df570_39c1212f","line":16,"in_reply_to":"da9df570_ce713279","updated":"2014-09-22 15:29:46.000000000","message":"Done","commit_id":"c8d20a37ec4e7bbfef05c45d62233a1cb90709f0"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"ebbc92030db6c3008ba2eb6cc86c464eb040e050","unresolved":false,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"class Process(subprocess.Popen):"},{"line_number":53,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":54,"context_line":"        stdin \u003d kwargs.pop(\u0027stdin\u0027, subprocess.PIPE)"},{"line_number":55,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"},{"line_number":56,"context_line":"        stderr \u003d kwargs.pop(\u0027stderr\u0027, subprocess.PIPE)"},{"line_number":57,"context_line":"        namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"}],"source_content_type":"text/x-python","patch_set":2,"id":"da9df570_ee6ef695","line":54,"updated":"2014-09-21 00:18:49.000000000","message":"put these in the function definition since you are not passing them on. It\u0027s much easier to read if the first 5 lines are just defining default args to the function.","commit_id":"c8d20a37ec4e7bbfef05c45d62233a1cb90709f0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"25cda7a095e0abb7af2a528f953e25ed73d96762","unresolved":false,"context_lines":[{"line_number":51,"context_line":""},{"line_number":52,"context_line":"class Process(subprocess.Popen):"},{"line_number":53,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":54,"context_line":"        stdin \u003d kwargs.pop(\u0027stdin\u0027, subprocess.PIPE)"},{"line_number":55,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"},{"line_number":56,"context_line":"        stderr \u003d kwargs.pop(\u0027stderr\u0027, subprocess.PIPE)"},{"line_number":57,"context_line":"        namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"}],"source_content_type":"text/x-python","patch_set":2,"id":"da9df570_f9cdd95e","line":54,"in_reply_to":"da9df570_ee6ef695","updated":"2014-09-22 15:29:46.000000000","message":"I agree it is easier but I wanted to make this class extending Popen constructor and making it with default value will break the order of parameters. e.g. bufsize won\u0027t be the first parameter anymore.","commit_id":"c8d20a37ec4e7bbfef05c45d62233a1cb90709f0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"8bcbcd91c43c2b17e4ce48cf666395188a1df43f","unresolved":false,"context_lines":[{"line_number":45,"context_line":""},{"line_number":46,"context_line":"cfg.CONF.register_opts(OPTS)"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"POLL_INTERVAL \u003d 3"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"class Process(subprocess.Popen):"}],"source_content_type":"text/x-python","patch_set":3,"id":"da9df570_bd86af11","line":48,"updated":"2014-09-23 13:57:57.000000000","message":"This is not used!","commit_id":"a2cdecf0553aee23808743c4afd05b440ea82f73"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"6af1155e1e5cb1265f7149d4e559a5af1718e7f4","unresolved":false,"context_lines":[{"line_number":48,"context_line":"POLL_INTERVAL \u003d 3"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"class Process(subprocess.Popen):"},{"line_number":52,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":53,"context_line":"        stdin \u003d kwargs.pop(\u0027stdin\u0027, subprocess.PIPE)"},{"line_number":54,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"}],"source_content_type":"text/x-python","patch_set":3,"id":"da9df570_0f2141fb","line":51,"updated":"2014-09-23 13:10:10.000000000","message":"Docstring for class?","commit_id":"a2cdecf0553aee23808743c4afd05b440ea82f73"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"8bcbcd91c43c2b17e4ce48cf666395188a1df43f","unresolved":false,"context_lines":[{"line_number":48,"context_line":"POLL_INTERVAL \u003d 3"},{"line_number":49,"context_line":""},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"class Process(subprocess.Popen):"},{"line_number":52,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":53,"context_line":"        stdin \u003d kwargs.pop(\u0027stdin\u0027, subprocess.PIPE)"},{"line_number":54,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"}],"source_content_type":"text/x-python","patch_set":3,"id":"da9df570_9d9acb61","line":51,"in_reply_to":"da9df570_0f2141fb","updated":"2014-09-23 13:57:57.000000000","message":"I think the code is straight forward.","commit_id":"a2cdecf0553aee23808743c4afd05b440ea82f73"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"5fefd9fc7676b06b950f0f00d7959c8763a9b6fc","unresolved":false,"context_lines":[{"line_number":52,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"},{"line_number":53,"context_line":"        stderr \u003d kwargs.pop(\u0027stderr\u0027, subprocess.PIPE)"},{"line_number":54,"context_line":"        self.namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"},{"line_number":55,"context_line":"        self.root_helper \u003d kwargs.pop(\u0027root_helper\u0027, None)"},{"line_number":56,"context_line":"        self.cmd \u003d cmd"},{"line_number":57,"context_line":"        if self.namespace is not None:"},{"line_number":58,"context_line":"            cmd \u003d [\u0027ip\u0027, \u0027netns\u0027, \u0027exec\u0027, self.namespace] + cmd"}],"source_content_type":"text/x-python","patch_set":4,"id":"da9df570_00c57d3b","line":55,"updated":"2014-09-24 01:27:40.000000000","message":"It doesn\u0027t make much sense to use kwargs instead of default arguments if you don\u0027t pass them on to a following function by popping them off. Using kwargs makes the top of the function busy setting up what should have happened in the function definition. Unless there is something I\u0027m missing, please remove these.","commit_id":"873032b0d20ea97cfab914c9c6ab2c6f0d16d149"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6e173e4abc8b870bd10a92c1b13df20c5c8dab3b","unresolved":false,"context_lines":[{"line_number":52,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"},{"line_number":53,"context_line":"        stderr \u003d kwargs.pop(\u0027stderr\u0027, subprocess.PIPE)"},{"line_number":54,"context_line":"        self.namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"},{"line_number":55,"context_line":"        self.root_helper \u003d kwargs.pop(\u0027root_helper\u0027, None)"},{"line_number":56,"context_line":"        self.cmd \u003d cmd"},{"line_number":57,"context_line":"        if self.namespace is not None:"},{"line_number":58,"context_line":"            cmd \u003d [\u0027ip\u0027, \u0027netns\u0027, \u0027exec\u0027, self.namespace] + cmd"}],"source_content_type":"text/x-python","patch_set":4,"id":"da9df570_2b0ed98d","line":55,"in_reply_to":"da9df570_00c57d3b","updated":"2014-09-24 11:18:19.000000000","message":"I replied in previous patchset. If I would use default arguments I would break order of parameters. e.g.\n\n def __init__(self, cmd, stdin\u003dsubprocess.PIPE, ...):\n\nwould lead to difference in usage of Process and Popen:\n\n popen \u003d Popen([\u0027foo\u0027], 1)      # spawn process with bufsize\u003d1\n process \u003d Process([\u0027foo\u0027], 1) # spawn process with stdin\u003d1","commit_id":"873032b0d20ea97cfab914c9c6ab2c6f0d16d149"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"060767c0b03edd30f9aa58f96f1d8b9c4824bf5e","unresolved":false,"context_lines":[{"line_number":52,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"},{"line_number":53,"context_line":"        stderr \u003d kwargs.pop(\u0027stderr\u0027, subprocess.PIPE)"},{"line_number":54,"context_line":"        self.namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"},{"line_number":55,"context_line":"        self.root_helper \u003d kwargs.pop(\u0027root_helper\u0027, None)"},{"line_number":56,"context_line":"        self.cmd \u003d cmd"},{"line_number":57,"context_line":"        if self.namespace is not None:"},{"line_number":58,"context_line":"            cmd \u003d [\u0027ip\u0027, \u0027netns\u0027, \u0027exec\u0027, self.namespace] + cmd"}],"source_content_type":"text/x-python","patch_set":4,"id":"da9df570_f3be7fd3","line":55,"in_reply_to":"da9df570_2b0ed98d","updated":"2014-09-24 18:22:07.000000000","message":"Oh, I see we are dealing with a python2 limitation of no default keyword arguments after *args. I would still rather be explicit about trying to match the order of the parent so some other refactor doesn\u0027t wipe this out.\n\n\ndef __init__(self, cmd, bufsize\u003d0, executable\u003dNone, stdin\u003dsubprocess.PIPE, ...):\n\n    super(Process, self).__init__(cmd, bufsize, executable, stdin\u003dstdin...\n\n\nEither that or be more explicit that defaults are just being set by avoiding the assignment to a local variable since you don\u0027t do anything with it other than pass it on.\n\n\ndef __init__(self, cmd, *args, **kwargs):\n    # change defaults from Popen\n    for arg in (\u0027stdin\u0027, \u0027stdout\u0027, \u0027stderr\u0027):\n        kwargs[arg] \u003d kwargs.get(arg, subprocess.PIPE)\n    ...\n    super(Process, self).__init__(cmd, *args, **kwargs)","commit_id":"873032b0d20ea97cfab914c9c6ab2c6f0d16d149"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"bca0e54e76648a76641fe833fd5ea6522f163141","unresolved":false,"context_lines":[{"line_number":52,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"},{"line_number":53,"context_line":"        stderr \u003d kwargs.pop(\u0027stderr\u0027, subprocess.PIPE)"},{"line_number":54,"context_line":"        self.namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"},{"line_number":55,"context_line":"        self.root_helper \u003d kwargs.pop(\u0027root_helper\u0027, None)"},{"line_number":56,"context_line":"        self.cmd \u003d cmd"},{"line_number":57,"context_line":"        if self.namespace is not None:"},{"line_number":58,"context_line":"            cmd \u003d [\u0027ip\u0027, \u0027netns\u0027, \u0027exec\u0027, self.namespace] + cmd"}],"source_content_type":"text/x-python","patch_set":4,"id":"da9df570_eb1036aa","line":55,"in_reply_to":"da9df570_f3be7fd3","updated":"2014-09-25 06:20:27.000000000","message":"Done","commit_id":"873032b0d20ea97cfab914c9c6ab2c6f0d16d149"},{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"20f2567d4d615de6e048cf55983b4b5fb2748981","unresolved":false,"context_lines":[{"line_number":50,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":51,"context_line":"        stdin \u003d kwargs.pop(\u0027stdin\u0027, subprocess.PIPE)"},{"line_number":52,"context_line":"        stdout \u003d kwargs.pop(\u0027stdout\u0027, subprocess.PIPE)"},{"line_number":53,"context_line":"        stderr \u003d kwargs.pop(\u0027stderr\u0027, subprocess.PIPE)"},{"line_number":54,"context_line":"        self.namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"},{"line_number":55,"context_line":"        self.root_helper \u003d kwargs.pop(\u0027root_helper\u0027, None)"},{"line_number":56,"context_line":"        self.cmd \u003d cmd"}],"source_content_type":"text/x-python","patch_set":5,"id":"da9df570_f393df4c","line":53,"updated":"2014-09-24 18:25:35.000000000","message":"I left a comment on PS4 about this. I think you need to be a lot more explicit about why this is done this way to prevent it from being changed in a refactor. I didn\u0027t realize until you explicitly told me that it was done with way to preserve order of a variable we can\u0027t even see in this method.","commit_id":"6198233a1dac83c454696395e0ea7ce9dd3b132c"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"9a77e73bbfe46b2f4b31f32ea38e4e34f4d70a33","unresolved":false,"context_lines":[{"line_number":71,"context_line":"                            \u0027that is no longer running.\u0027), {\u0027cmd\u0027: self.cmd,"},{"line_number":72,"context_line":"                                                            \u0027pid\u0027: self.pid})"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def wait_for_child_process(self, timeout\u003d1, sleep\u003d0.1):"},{"line_number":75,"context_line":"        with eventlet.timeout.Timeout(timeout):"},{"line_number":76,"context_line":"            while not utils.find_child_pids(str(self.pid)):"},{"line_number":77,"context_line":"                eventlet.sleep(sleep)"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_ecbb4470","line":74,"updated":"2014-09-30 13:19:23.000000000","message":"I know that these kind of timeouts can cause non-deterministic failures in the gate in cases where the gate is really busy. While this seems like a fat chance that it will take longer than one second to do some \u0027ps\u0027 processes, I think we\u0027ll be surprised some day. I strongly suggest this timeout here be increased to 10/15 seconds.\n\nOther than that, maybe consider putting this default value as a configurable value (in OPTS above)?","commit_id":"8ee58af4b91c0ced5dee0509712bc39b893d482d"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"729ebf1089d5ecdfdde6437dfa5dac921af59a4b","unresolved":false,"context_lines":[{"line_number":71,"context_line":"                            \u0027that is no longer running.\u0027), {\u0027cmd\u0027: self.cmd,"},{"line_number":72,"context_line":"                                                            \u0027pid\u0027: self.pid})"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def wait_for_child_process(self, timeout\u003d1, sleep\u003d0.1):"},{"line_number":75,"context_line":"        with eventlet.timeout.Timeout(timeout):"},{"line_number":76,"context_line":"            while not utils.find_child_pids(str(self.pid)):"},{"line_number":77,"context_line":"                eventlet.sleep(sleep)"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_5e73d9b0","line":74,"in_reply_to":"baa201ad_ecbb4470","updated":"2014-10-02 14:07:44.000000000","message":"You\u0027re right, I will need to change the defaults as I was testing it on my low loaded environment.","commit_id":"8ee58af4b91c0ced5dee0509712bc39b893d482d"}],"neutron/agent/linux/utils.py":[{"author":{"_account_id":4187,"name":"Ryan Tidwell","email":"rtidwell@suse.com","username":"ryan-tidwell"},"change_message_id":"8c3796a4dbca670226d9e5a1094d60c2a4859b9a","unresolved":false,"context_lines":[{"line_number":195,"context_line":"            # Process is already dead"},{"line_number":196,"context_line":"            return None"},{"line_number":197,"context_line":"        while True:"},{"line_number":198,"context_line":"            try:"},{"line_number":199,"context_line":"                # We shouldn\u0027t have more than one child per process"},{"line_number":200,"context_line":"                # so keep getting the children of the first one"},{"line_number":201,"context_line":"                pid \u003d find_child_pids(pid)[0]"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa98f980_bc4c07ca","line":198,"updated":"2014-09-17 18:20:45.000000000","message":"Can this be done without try/except?","commit_id":"c45fdcfe5773001d6b9598637976a942a40dc9d4"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"015dda7da51f7c7d60dc7bd1fae9a9aef43d9d54","unresolved":false,"context_lines":[{"line_number":195,"context_line":"            # Process is already dead"},{"line_number":196,"context_line":"            return None"},{"line_number":197,"context_line":"        while True:"},{"line_number":198,"context_line":"            try:"},{"line_number":199,"context_line":"                # We shouldn\u0027t have more than one child per process"},{"line_number":200,"context_line":"                # so keep getting the children of the first one"},{"line_number":201,"context_line":"                pid \u003d find_child_pids(pid)[0]"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa98f980_2a2ef78c","line":198,"in_reply_to":"fa98f980_bc4c07ca","updated":"2014-09-18 08:41:08.000000000","message":"It can. The code is taken from AsyncProcess and changing it is out of scope of this patch. Anyway I think removing try block won\u0027t give any benefit.","commit_id":"c45fdcfe5773001d6b9598637976a942a40dc9d4"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"6af1155e1e5cb1265f7149d4e559a5af1718e7f4","unresolved":false,"context_lines":[{"line_number":189,"context_line":"    # running, re-parented to init, so the only way to ensure that both"},{"line_number":190,"context_line":"    # die is to target the child process directly."},{"line_number":191,"context_line":"    if root_helper is not None:"},{"line_number":192,"context_line":"        try:"},{"line_number":193,"context_line":"            pid \u003d find_child_pids(pid)[0]"},{"line_number":194,"context_line":"        except IndexError:"},{"line_number":195,"context_line":"            # Process is already dead"}],"source_content_type":"text/x-python","patch_set":3,"id":"da9df570_0ffdc13a","line":192,"updated":"2014-09-23 13:10:10.000000000","message":"Could this be combined into one loop?\n    last_pid \u003d None\n    while True:\n        try:\n            last_pid \u003d find_child_pids(pid)[0]\n            pid \u003d last_pid\n        except IndexError:\n            break\n    return last_pid","commit_id":"a2cdecf0553aee23808743c4afd05b440ea82f73"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"8bcbcd91c43c2b17e4ce48cf666395188a1df43f","unresolved":false,"context_lines":[{"line_number":189,"context_line":"    # running, re-parented to init, so the only way to ensure that both"},{"line_number":190,"context_line":"    # die is to target the child process directly."},{"line_number":191,"context_line":"    if root_helper is not None:"},{"line_number":192,"context_line":"        try:"},{"line_number":193,"context_line":"            pid \u003d find_child_pids(pid)[0]"},{"line_number":194,"context_line":"        except IndexError:"},{"line_number":195,"context_line":"            # Process is already dead"}],"source_content_type":"text/x-python","patch_set":3,"id":"da9df570_9db56bf0","line":192,"in_reply_to":"da9df570_0ffdc13a","updated":"2014-09-23 13:57:57.000000000","message":"Thanks for suggestion. I think refactoring of this function is out of scope of this patch.","commit_id":"a2cdecf0553aee23808743c4afd05b440ea82f73"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"66af809157c604407ecb09796d830fce1c6902a0","unresolved":false,"context_lines":[{"line_number":189,"context_line":"    # running, re-parented to init, so the only way to ensure that both"},{"line_number":190,"context_line":"    # die is to target the child process directly."},{"line_number":191,"context_line":"    if root_helper is not None:"},{"line_number":192,"context_line":"        try:"},{"line_number":193,"context_line":"            pid \u003d find_child_pids(pid)[0]"},{"line_number":194,"context_line":"        except IndexError:"},{"line_number":195,"context_line":"            # Process is already dead"}],"source_content_type":"text/x-python","patch_set":3,"id":"da9df570_39371350","line":192,"in_reply_to":"da9df570_99363fe6","updated":"2014-09-24 12:36:50.000000000","message":"It\u0027s not a new function, it\u0027s moved out AsyncProcess class so it can be used on other places: https://review.openstack.org/#/c/122207/4/neutron/agent/linux/async_process.py","commit_id":"a2cdecf0553aee23808743c4afd05b440ea82f73"},{"author":{"_account_id":6659,"name":"Paul Michali","email":"pc@michali.net","username":"pcm"},"change_message_id":"72189c66c101281d7c257548088ffcdcafe72f6a","unresolved":false,"context_lines":[{"line_number":189,"context_line":"    # running, re-parented to init, so the only way to ensure that both"},{"line_number":190,"context_line":"    # die is to target the child process directly."},{"line_number":191,"context_line":"    if root_helper is not None:"},{"line_number":192,"context_line":"        try:"},{"line_number":193,"context_line":"            pid \u003d find_child_pids(pid)[0]"},{"line_number":194,"context_line":"        except IndexError:"},{"line_number":195,"context_line":"            # Process is already dead"}],"source_content_type":"text/x-python","patch_set":3,"id":"da9df570_99363fe6","line":192,"in_reply_to":"da9df570_9db56bf0","updated":"2014-09-24 12:13:06.000000000","message":"It\u0027s a new method added. Why is refactoring out of scope?","commit_id":"a2cdecf0553aee23808743c4afd05b440ea82f73"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":128,"context_line":"        # Unexpected errors are the responsibility of the caller"},{"line_number":129,"context_line":"        with excutils.save_and_reraise_exception() as ctxt:"},{"line_number":130,"context_line":"            # Exception has already been logged by execute"},{"line_number":131,"context_line":"            no_children_found \u003d \u0027Exit code: 1\u0027 in e.message"},{"line_number":132,"context_line":"            if no_children_found:"},{"line_number":133,"context_line":"                ctxt.reraise \u003d False"},{"line_number":134,"context_line":"                return []"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_a6ef30b0","line":131,"updated":"2014-10-15 11:31:16.000000000","message":"Optimally the error thrown from execute should have more fields, so you can just do e.return_code and get the integer \u00271\u0027, but that\u0027s really out of the scope of this code. Maybe I can write it in a future patch?","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":128,"context_line":"        # Unexpected errors are the responsibility of the caller"},{"line_number":129,"context_line":"        with excutils.save_and_reraise_exception() as ctxt:"},{"line_number":130,"context_line":"            # Exception has already been logged by execute"},{"line_number":131,"context_line":"            no_children_found \u003d \u0027Exit code: 1\u0027 in e.message"},{"line_number":132,"context_line":"            if no_children_found:"},{"line_number":133,"context_line":"                ctxt.reraise \u003d False"},{"line_number":134,"context_line":"                return []"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_873866d4","line":131,"in_reply_to":"baa201ad_a6ef30b0","updated":"2014-10-15 12:53:33.000000000","message":"Yes, it\u0027s out of scope of this patch. This is only to avoid errors in logs in case we hit the child pid and message in exception is localized.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        os.unlink(conf_file)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    # If root helper was used, two or more processes will be created:"},{"line_number":186,"context_line":"    #"},{"line_number":187,"context_line":"    #  - a root helper process (e.g. sudo myscript)"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_e6b778d6","line":184,"updated":"2014-10-15 11:31:16.000000000","message":"The name of the function and the comments below create somewhat of a conflict: If the only purpose of the function is to be used for detecting the child rootwrap, then the function\u0027s name should be changed, and if the function should be generic (as it should), then the comment is very specific for your use case.\n\nI propose:\n\n1. Renaming the function to \"get_deepest_child_pid\" or something similar (no real good name comes to mind right now)\n\n2. Change the comment into a docstring, and use the use-case of rootwrap as an example use-case.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"5ddea35d83f6ac6252b43fe9c80e6e996829926e","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        os.unlink(conf_file)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    # If root helper was used, two or more processes will be created:"},{"line_number":186,"context_line":"    #"},{"line_number":187,"context_line":"    #  - a root helper process (e.g. sudo myscript)"}],"source_content_type":"text/x-python","patch_set":8,"id":"9aa7fdbe_549ce369","line":184,"in_reply_to":"9aa7fdbe_fb4ddac5","updated":"2014-10-20 15:45:18.000000000","message":"Done","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"c353af4c869e299f644656782818bb8ff84b2c8d","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        os.unlink(conf_file)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    # If root helper was used, two or more processes will be created:"},{"line_number":186,"context_line":"    #"},{"line_number":187,"context_line":"    #  - a root helper process (e.g. sudo myscript)"}],"source_content_type":"text/x-python","patch_set":8,"id":"9aa7fdbe_fb4ddac5","line":184,"in_reply_to":"baa201ad_077b968c","updated":"2014-10-19 10:58:20.000000000","message":"At a minimum the comment needs to be changed to a docstring IMO. If you don\u0027t like \u0027get_deepest_child_pid\u0027 i\u0027m not gonna force it (especially since I don\u0027t like it either).","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        os.unlink(conf_file)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    # If root helper was used, two or more processes will be created:"},{"line_number":186,"context_line":"    #"},{"line_number":187,"context_line":"    #  - a root helper process (e.g. sudo myscript)"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_077b968c","line":184,"in_reply_to":"baa201ad_e6b778d6","updated":"2014-10-15 12:53:33.000000000","message":"I\u0027d rather stick to original. Do you think keeping the original name would make more sense?","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":208,"context_line":"    return pid"},{"line_number":209,"context_line":""},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"def remove_abs_path(cmd):"},{"line_number":212,"context_line":"    if os.path.isabs(cmd[0]):"},{"line_number":213,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":214,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_06dabc4b","line":211,"updated":"2014-10-15 11:31:16.000000000","message":"This function needs some documentation - what\u0027s the format of cmd? is it a string? a list of strings?","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":208,"context_line":"    return pid"},{"line_number":209,"context_line":""},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"def remove_abs_path(cmd):"},{"line_number":212,"context_line":"    if os.path.isabs(cmd[0]):"},{"line_number":213,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":214,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_8766a6eb","line":211,"in_reply_to":"baa201ad_06dabc4b","updated":"2014-10-15 12:53:33.000000000","message":"Done","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":""},{"line_number":216,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":217,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":218,"context_line":"        return False"},{"line_number":219,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_d8009304","line":216,"updated":"2014-10-15 11:31:16.000000000","message":"This seems rather similar to functions present at neutron.agent.linux.daemon, neutron.agent.linux.dhcp and neutron.agent.linux.external_process. What do you think about changing those so it will use this one?","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"5ddea35d83f6ac6252b43fe9c80e6e996829926e","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":""},{"line_number":216,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":217,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":218,"context_line":"        return False"},{"line_number":219,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"}],"source_content_type":"text/x-python","patch_set":8,"id":"9aa7fdbe_b4998f70","line":216,"in_reply_to":"9aa7fdbe_1b4b7eb1","updated":"2014-10-20 15:45:18.000000000","message":"I agree.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":""},{"line_number":216,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":217,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":218,"context_line":"        return False"},{"line_number":219,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_e72db247","line":216,"in_reply_to":"baa201ad_d8009304","updated":"2014-10-15 12:53:33.000000000","message":"I think refactoring of those modules is above the scope of this patch.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"c353af4c869e299f644656782818bb8ff84b2c8d","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":""},{"line_number":216,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":217,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":218,"context_line":"        return False"},{"line_number":219,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"}],"source_content_type":"text/x-python","patch_set":8,"id":"9aa7fdbe_1b4b7eb1","line":216,"in_reply_to":"baa201ad_e72db247","updated":"2014-10-19 10:58:20.000000000","message":"Yes, but regardless of that, many a future patch could deal with it (not asking you to do it ;-) ). What do you think?","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":217,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":218,"context_line":"        return False"},{"line_number":219,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":220,"context_line":"        cmdline \u003d f.readline().split(\u0027\\0\u0027)[:-1]"},{"line_number":221,"context_line":"    remove_abs_path(cmd)"},{"line_number":222,"context_line":"    remove_abs_path(cmdline)"},{"line_number":223,"context_line":"    return cmdline \u003d\u003d cmd"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_861eac0a","line":220,"updated":"2014-10-15 11:31:16.000000000","message":"I assume the last character which you are removing is usually a \u0027\\n\u0027? If so, i think \u0027strip\u0027 will work just fine here.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"40129da7f0a4048c4e2b2fe3fb05e5bae00623f7","unresolved":false,"context_lines":[{"line_number":217,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":218,"context_line":"        return False"},{"line_number":219,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":220,"context_line":"        cmdline \u003d f.readline().split(\u0027\\0\u0027)[:-1]"},{"line_number":221,"context_line":"    remove_abs_path(cmd)"},{"line_number":222,"context_line":"    remove_abs_path(cmdline)"},{"line_number":223,"context_line":"    return cmdline \u003d\u003d cmd"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_fdb0c58c","line":220,"in_reply_to":"baa201ad_6c2609d0","updated":"2014-10-16 11:20:34.000000000","message":"Actually zero is a separator for parameters.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":217,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":218,"context_line":"        return False"},{"line_number":219,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":220,"context_line":"        cmdline \u003d f.readline().split(\u0027\\0\u0027)[:-1]"},{"line_number":221,"context_line":"    remove_abs_path(cmd)"},{"line_number":222,"context_line":"    remove_abs_path(cmdline)"},{"line_number":223,"context_line":"    return cmdline \u003d\u003d cmd"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_6c2609d0","line":220,"in_reply_to":"baa201ad_861eac0a","updated":"2014-10-15 12:53:33.000000000","message":"Done","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":13538,"name":"Claudiu Popa","email":"cpopa@cloudbasesolutions.com","username":"PCManticore"},"change_message_id":"3b086189e082e6552d6e8315924b1cb62e246c1c","unresolved":false,"context_lines":[{"line_number":222,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":223,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":224,"context_line":"        return False"},{"line_number":225,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":226,"context_line":"        cmdline \u003d [f.readline().strip(\u0027\\0\u0027)]"},{"line_number":227,"context_line":"    remove_abs_path(cmd)"},{"line_number":228,"context_line":"    remove_abs_path(cmdline)"}],"source_content_type":"text/x-python","patch_set":10,"id":"baa201ad_0d33b414","line":225,"updated":"2014-10-15 15:38:28.000000000","message":"Use \u0027open\u0027 instead of \u0027file\u0027.","commit_id":"75a8c83fccb7bc0c6d4e3ab2e782bc0498d79f8e"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d3f3d760c70f4f2187376b196d993a9e0a993c35","unresolved":false,"context_lines":[{"line_number":222,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":223,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":224,"context_line":"        return False"},{"line_number":225,"context_line":"    with file(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":226,"context_line":"        cmdline \u003d [f.readline().strip(\u0027\\0\u0027)]"},{"line_number":227,"context_line":"    remove_abs_path(cmd)"},{"line_number":228,"context_line":"    remove_abs_path(cmdline)"}],"source_content_type":"text/x-python","patch_set":10,"id":"baa201ad_bd38fdee","line":225,"in_reply_to":"baa201ad_0d33b414","updated":"2014-10-16 11:26:02.000000000","message":"Oops, thanks for noticing :)","commit_id":"75a8c83fccb7bc0c6d4e3ab2e782bc0498d79f8e"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"734045ecf7ed7e52e5157da80e57e2212bf7c531","unresolved":false,"context_lines":[{"line_number":201,"context_line":"            try:"},{"line_number":202,"context_line":"                # We shouldn\u0027t have more than one child per process"},{"line_number":203,"context_line":"                # so keep getting the children of the first one"},{"line_number":204,"context_line":"                pid \u003d find_child_pids(pid)[0]"},{"line_number":205,"context_line":"            except IndexError:"},{"line_number":206,"context_line":"                # Last process in the tree, return it"},{"line_number":207,"context_line":"                break"}],"source_content_type":"text/x-python","patch_set":11,"id":"baa201ad_f8b0d08c","line":204,"updated":"2014-10-16 13:45:32.000000000","message":"Do you keep looking \"recursively\" ? \n\nWill that work for example for a \"dhcp-agent\" which will also spawn dnsmasq\u0027s?\n\nMay be it\u0027s worth looking for the cmdline of the pids, to verify, if it\u0027s sudo: get next, if it\u0027s root-wrap, get next... otherwise, stop.\n\n/proc/\u003cpid\u003e/cmdline","commit_id":"bfe13c5e9a1aa91792c87f5df3632c4fa2f0545b"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d6faf59d00486b8c227557ba9328f9e10bf2f140","unresolved":false,"context_lines":[{"line_number":201,"context_line":"            try:"},{"line_number":202,"context_line":"                # We shouldn\u0027t have more than one child per process"},{"line_number":203,"context_line":"                # so keep getting the children of the first one"},{"line_number":204,"context_line":"                pid \u003d find_child_pids(pid)[0]"},{"line_number":205,"context_line":"            except IndexError:"},{"line_number":206,"context_line":"                # Last process in the tree, return it"},{"line_number":207,"context_line":"                break"}],"source_content_type":"text/x-python","patch_set":11,"id":"baa201ad_6e4eda70","line":204,"in_reply_to":"baa201ad_f8b0d08c","updated":"2014-10-16 14:12:02.000000000","message":"This function is just taken from async_process. It looks iteratively  using \u0027ps --ppid\u0027, so if dhcp-agent spawns dnsmasqs, it will return first dnsmasq process.","commit_id":"bfe13c5e9a1aa91792c87f5df3632c4fa2f0545b"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    try:"},{"line_number":125,"context_line":"        raw_pids \u003d execute([\u0027ps\u0027, \u0027--ppid\u0027, pid, \u0027-o\u0027, \u0027pid\u003d\u0027],"},{"line_number":126,"context_line":"                           log_fail_as_error\u003dFalse)"},{"line_number":127,"context_line":"    except RuntimeError as e:"},{"line_number":128,"context_line":"        # Unexpected errors are the responsibility of the caller"},{"line_number":129,"context_line":"        with excutils.save_and_reraise_exception() as ctxt:"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_5e7654d8","line":126,"updated":"2014-10-27 17:17:20.000000000","message":"Good catch.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        os.unlink(conf_file)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_de8084c2","line":184,"updated":"2014-10-27 17:17:20.000000000","message":"I think the name of this method needs to be more explicit given how specialized it is.  It\u0027s not just getting the child pid of the given pid - it\u0027s getting the first child of those lowest in the process hierarchy.  Since the functionality is targeted at rootwrap-invoked processes, maybe something like \u0027get_roothelper_child_pid\u0027 makes more sense?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"f80561bc776bc5e9853f13cd653918ca320cb9f9","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        os.unlink(conf_file)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_42bd4001","line":184,"in_reply_to":"9aa7fdbe_de8084c2","updated":"2014-10-28 07:54:19.000000000","message":"+1","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        os.unlink(conf_file)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_7dbaace2","line":184,"in_reply_to":"9aa7fdbe_de8084c2","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"066e55145548838eebd78be566bc6c68935efe02","unresolved":false,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"    If root helper was used, two or more processes will be created:"},{"line_number":189,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_bcfe68ac","line":186,"updated":"2014-10-23 15:38:21.000000000","message":"nit: \"Get the deepest child process\" is a good docstring here I think. Also move the first line up:\n\n \"\"\"Get the childest...","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"    If root helper was used, two or more processes will be created:"},{"line_number":189,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_bdb414f3","line":186,"in_reply_to":"9aa7fdbe_0c3e21da","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"79e7b002885be3ae39dc7cb0e00a2387632bc514","unresolved":false,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"    If root helper was used, two or more processes will be created:"},{"line_number":189,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_0c3e21da","line":186,"in_reply_to":"9aa7fdbe_a3db6d7a","updated":"2014-10-27 15:53:28.000000000","message":"+1 ;-)","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0a58d8822d0eecbd0e5589d170a719da541f11c5","unresolved":false,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"def get_child_pid(pid, root_helper\u003dNone):"},{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"    If root helper was used, two or more processes will be created:"},{"line_number":189,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_a3db6d7a","line":186,"in_reply_to":"9aa7fdbe_bcfe68ac","updated":"2014-10-27 11:27:06.000000000","message":"ok, I\u0027ll stop inventing my own words :)","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"    If root helper was used, two or more processes will be created:"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"     - a root helper process (e.g. sudo myscript)"},{"line_number":191,"context_line":"     - possibly a rootwrap script (e.g. neutron-rootwrap)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_3e6fd02c","line":188,"updated":"2014-10-27 17:17:20.000000000","message":"will -\u003e would","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":185,"context_line":"    \"\"\""},{"line_number":186,"context_line":"    Get the childest process in the process tree"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"    If root helper was used, two or more processes will be created:"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"     - a root helper process (e.g. sudo myscript)"},{"line_number":191,"context_line":"     - possibly a rootwrap script (e.g. neutron-rootwrap)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_5d51760d","line":188,"in_reply_to":"9aa7fdbe_3e6fd02c","updated":"2014-11-11 17:05:34.000000000","message":"I kept wording how Maru wrote it ;)\nDone","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":195,"context_line":"    running, re-parented to init, so the only way to ensure that both"},{"line_number":196,"context_line":"    die is to target the child process directly."},{"line_number":197,"context_line":"    \"\"\""},{"line_number":198,"context_line":"    if root_helper is not None:"},{"line_number":199,"context_line":"        try:"},{"line_number":200,"context_line":"            pid \u003d find_child_pids(pid)[0]"},{"line_number":201,"context_line":"        except IndexError:"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_5eaf342a","line":198,"updated":"2014-10-27 17:17:20.000000000","message":"(No action required) Is there a reason not to continue to use implicit boolean evaluation?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":195,"context_line":"    running, re-parented to init, so the only way to ensure that both"},{"line_number":196,"context_line":"    die is to target the child process directly."},{"line_number":197,"context_line":"    \"\"\""},{"line_number":198,"context_line":"    if root_helper is not None:"},{"line_number":199,"context_line":"        try:"},{"line_number":200,"context_line":"            pid \u003d find_child_pids(pid)[0]"},{"line_number":201,"context_line":"        except IndexError:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_1da0c88f","line":198,"in_reply_to":"9aa7fdbe_5eaf342a","updated":"2014-11-11 17:05:34.000000000","message":"You\u0027re right, this change doesn\u0027t belong to this patch.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"cfef5bb98f2ed93fbf745a638bc0229990c66a21","unresolved":false,"context_lines":[{"line_number":213,"context_line":""},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"def remove_abs_path(cmd):"},{"line_number":216,"context_line":"    \"\"\"In case executable in cmd is absolue path it\u0027s replaced just by"},{"line_number":217,"context_line":"    executable"},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"    :param cmd: parsed shlex command (e.g. [\u0027/bin/foo\u0027, \u0027param1\u0027, \u0027param two\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_510fec87","line":216,"updated":"2014-10-27 10:23:39.000000000","message":"nit: absolue-\u003eabsolute","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0a58d8822d0eecbd0e5589d170a719da541f11c5","unresolved":false,"context_lines":[{"line_number":213,"context_line":""},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"def remove_abs_path(cmd):"},{"line_number":216,"context_line":"    \"\"\"In case executable in cmd is absolue path it\u0027s replaced just by"},{"line_number":217,"context_line":"    executable"},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"    :param cmd: parsed shlex command (e.g. [\u0027/bin/foo\u0027, \u0027param1\u0027, \u0027param two\u0027])"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_e3e1f52a","line":216,"in_reply_to":"9aa7fdbe_510fec87","updated":"2014-10-27 11:27:06.000000000","message":"Good eye.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":223,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":""},{"line_number":226,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":227,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":228,"context_line":"        return False"},{"line_number":229,"context_line":"    with open(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_d7df870b","line":226,"updated":"2014-10-27 17:17:20.000000000","message":"I think this method might be better split - one method to retrieve the command line for the pid, and another that validates that the abs path of the commands.  But at the least, prefixing with \u0027check_\u0027 generally implies that an exception will be thrown when a desired condition is not true, so a name like \u0027pid_invoked_with_cmdline\u0027.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":223,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":""},{"line_number":226,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":227,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":228,"context_line":"        return False"},{"line_number":229,"context_line":"    with open(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_bd5a5481","line":226,"in_reply_to":"9aa7fdbe_d7df870b","updated":"2014-11-11 17:05:34.000000000","message":"Done, split into two and additional one calling both.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":226,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":227,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":228,"context_line":"        return False"},{"line_number":229,"context_line":"    with open(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":230,"context_line":"        cmdline \u003d f.readline().split(\u0027\\0\u0027)[:-1]"},{"line_number":231,"context_line":"    remove_abs_path(cmd)"},{"line_number":232,"context_line":"    remove_abs_path(cmdline)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_de1fa46a","line":229,"updated":"2014-10-27 17:17:20.000000000","message":"What if this file isn\u0027t readable or an io error occurs?  Do we want the error to propagate?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"acc88daf5358a336da07d97abeeca4cc7bb0ffab","unresolved":false,"context_lines":[{"line_number":226,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":227,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":228,"context_line":"        return False"},{"line_number":229,"context_line":"    with open(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":230,"context_line":"        cmdline \u003d f.readline().split(\u0027\\0\u0027)[:-1]"},{"line_number":231,"context_line":"    remove_abs_path(cmd)"},{"line_number":232,"context_line":"    remove_abs_path(cmdline)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_88a49d08","line":229,"in_reply_to":"9aa7fdbe_0275f862","updated":"2014-10-28 14:49:22.000000000","message":"Fair enough.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"f80561bc776bc5e9853f13cd653918ca320cb9f9","unresolved":false,"context_lines":[{"line_number":226,"context_line":"def check_pid_with_cmd(pid, cmd):"},{"line_number":227,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":228,"context_line":"        return False"},{"line_number":229,"context_line":"    with open(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":230,"context_line":"        cmdline \u003d f.readline().split(\u0027\\0\u0027)[:-1]"},{"line_number":231,"context_line":"    remove_abs_path(cmd)"},{"line_number":232,"context_line":"    remove_abs_path(cmdline)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_0275f862","line":229,"in_reply_to":"9aa7fdbe_de1fa46a","updated":"2014-10-28 07:54:19.000000000","message":"Actually all the files in /proc are managed by the kernel (ie. reading a file from the /proc filesystem triggers a kernel function which handles the specific request - and not the normal \"lets read from the HD\" type of function, but a \"lets retrieve some data from our internal data structures and return it to the user\"), so this file should always be readable (mod 0o444).\n\nIf an error is popped from here, I think something much worse is going on and we should consider whether or not the process should quit (sys.exit()) because of it.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"fd0df208fafeb8a159caaf3836fd2aff8d13867e","unresolved":false,"context_lines":[{"line_number":242,"context_line":"    return cmd1 \u003d\u003d cmd2"},{"line_number":243,"context_line":""},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"def pid_invoked_with_cmdline(pid, cmd):"},{"line_number":246,"context_line":"    \"\"\"Validate process with given pid is running with provided parameters"},{"line_number":247,"context_line":""},{"line_number":248,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_12cba51f","line":245,"updated":"2014-11-17 12:33:14.000000000","message":"nit: rename cmd to \u0027expected_cmd\u0027 to clarify the meaning?","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"b77bec89c9b59d1de62e1f253b662ee0e263612d","unresolved":false,"context_lines":[{"line_number":242,"context_line":"    return cmd1 \u003d\u003d cmd2"},{"line_number":243,"context_line":""},{"line_number":244,"context_line":""},{"line_number":245,"context_line":"def pid_invoked_with_cmdline(pid, cmd):"},{"line_number":246,"context_line":"    \"\"\"Validate process with given pid is running with provided parameters"},{"line_number":247,"context_line":""},{"line_number":248,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_f00dd538","line":245,"in_reply_to":"5a890539_12cba51f","updated":"2014-11-19 16:23:58.000000000","message":"Done","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"43cd980fdad6f7e4521f1cfcbdd2884b122643e4","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    return pid"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"def remove_abs_path(cmd):"},{"line_number":215,"context_line":"    \"\"\"In case executable in cmd is absolute path it\u0027s replaced just by"},{"line_number":216,"context_line":"    executable"},{"line_number":217,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"5a890539_051138af","line":214,"updated":"2014-11-28 10:36:32.000000000","message":"this method uses side-effects ... not sure it\u0027s a good idea or at least it should be documented","commit_id":"70b323dd919029662623cf254eb35aa1e6f26058"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"bf7e13c4b640fa08163c25124df7de5c76783f7d","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    return pid"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"def remove_abs_path(cmd):"},{"line_number":215,"context_line":"    \"\"\"In case executable in cmd is absolute path it\u0027s replaced just by"},{"line_number":216,"context_line":"    executable"},{"line_number":217,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"5a890539_d32ffa86","line":214,"in_reply_to":"5a890539_051138af","updated":"2014-11-28 11:13:45.000000000","message":"I think I don\u0027t follow. Can you be more specific, please?","commit_id":"70b323dd919029662623cf254eb35aa1e6f26058"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"9a89b1cc49ea5553ed0de89d49c8c793868d3f24","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    return pid"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"def remove_abs_path(cmd):"},{"line_number":215,"context_line":"    \"\"\"In case executable in cmd is absolute path it\u0027s replaced just by"},{"line_number":216,"context_line":"    executable"},{"line_number":217,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"5a890539_2e8291c8","line":214,"in_reply_to":"5a890539_6ead79ea","updated":"2014-11-28 12:29:34.000000000","message":"Thanks for explanation, I\u0027ll update it.","commit_id":"70b323dd919029662623cf254eb35aa1e6f26058"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"aa87f85ae8db080b2b91e19c1384a915444586c6","unresolved":false,"context_lines":[{"line_number":211,"context_line":"    return pid"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":""},{"line_number":214,"context_line":"def remove_abs_path(cmd):"},{"line_number":215,"context_line":"    \"\"\"In case executable in cmd is absolute path it\u0027s replaced just by"},{"line_number":216,"context_line":"    executable"},{"line_number":217,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"5a890539_6ead79ea","line":214,"in_reply_to":"5a890539_d32ffa86","updated":"2014-11-28 12:07:16.000000000","message":"cmd is modified by remove_abs_path despite cmd is an input. it implies non trivially behavior\n\n  backup \u003d cmd[:]\n  remove_abs_path(cmd)\n\n  # assert could fail despite it looks like cmd has not been updated\n  self.assertEqual(backup, cmd)\n\nIt\u0027s safer to ensure cmd is not modified by remove_abs_path and reaffects remove_abs_path result to cmd","commit_id":"70b323dd919029662623cf254eb35aa1e6f26058"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"8c19d848dc10eee35b70c00f73976c1a5da6ccde","unresolved":false,"context_lines":[{"line_number":221,"context_line":"    \"\"\""},{"line_number":222,"context_line":"    if cmd and os.path.isabs(cmd[0]):"},{"line_number":223,"context_line":"        cmd \u003d list(cmd)"},{"line_number":224,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":225,"context_line":""},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"def get_cmdline_from_pid(pid):"}],"source_content_type":"text/x-python","patch_set":19,"id":"5a890539_199fe1b5","line":224,"updated":"2014-11-28 12:50:00.000000000","message":"the following seems missing:\n\n  return cmd\n\n:)","commit_id":"b4496c7660eadf8340166c01e33941467d5bcd4a"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"c68e019d19339368afef850a87e88ecf656afc1f","unresolved":false,"context_lines":[{"line_number":221,"context_line":"    \"\"\""},{"line_number":222,"context_line":"    if cmd and os.path.isabs(cmd[0]):"},{"line_number":223,"context_line":"        cmd \u003d list(cmd)"},{"line_number":224,"context_line":"        cmd[0] \u003d os.path.basename(cmd[0])"},{"line_number":225,"context_line":""},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"def get_cmdline_from_pid(pid):"}],"source_content_type":"text/x-python","patch_set":19,"id":"5a890539_540db036","line":224,"in_reply_to":"5a890539_199fe1b5","updated":"2014-11-28 13:19:10.000000000","message":"Thanks!","commit_id":"b4496c7660eadf8340166c01e33941467d5bcd4a"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"5c02eb96f4b21805d426eb33adcfb91d23626c9c","unresolved":false,"context_lines":[{"line_number":200,"context_line":"        except IndexError:"},{"line_number":201,"context_line":"            # Process is already dead"},{"line_number":202,"context_line":"            return None"},{"line_number":203,"context_line":"        while True:"},{"line_number":204,"context_line":"            try:"},{"line_number":205,"context_line":"                # We shouldn\u0027t have more than one child per process"},{"line_number":206,"context_line":"                # so keep getting the children of the first one"}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_948f8c74","line":203,"updated":"2014-12-12 13:02:28.000000000","message":"you should exchange while and try blocks:\n\n  try:\n    while True:\n      pid \u003d ...\n  except IndexError\n    pass","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"737f937d14b97fddc10b7565106d4e7fcf902742","unresolved":false,"context_lines":[{"line_number":200,"context_line":"        except IndexError:"},{"line_number":201,"context_line":"            # Process is already dead"},{"line_number":202,"context_line":"            return None"},{"line_number":203,"context_line":"        while True:"},{"line_number":204,"context_line":"            try:"},{"line_number":205,"context_line":"                # We shouldn\u0027t have more than one child per process"},{"line_number":206,"context_line":"                # so keep getting the children of the first one"}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_ba171144","line":203,"in_reply_to":"3a961159_948f8c74","updated":"2014-12-12 13:40:17.000000000","message":"Good idea. I wasn\u0027t really looking on how was this function implemented, I just needed to separate it out of class.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"78ba0d9f4168a61f3d5768e66eb0916bcd2758bb","unresolved":false,"context_lines":[{"line_number":200,"context_line":"        except IndexError:"},{"line_number":201,"context_line":"            # Process is already dead"},{"line_number":202,"context_line":"            return None"},{"line_number":203,"context_line":"        while True:"},{"line_number":204,"context_line":"            try:"},{"line_number":205,"context_line":"                # We shouldn\u0027t have more than one child per process"},{"line_number":206,"context_line":"                # so keep getting the children of the first one"}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_6e7a4bbe","line":203,"in_reply_to":"3a961159_ba171144","updated":"2014-12-12 15:50:49.000000000","message":"I personally prefer to limit the scope of try blocks unless there is a good reason not to, and given that the \u0027try\u0027 block executes at most twice, optimization doesn\u0027t really buy anything.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"8088047ef7e113f1e794b908a3ca03c989c14aa5","unresolved":false,"context_lines":[{"line_number":203,"context_line":"        while True:"},{"line_number":204,"context_line":"            try:"},{"line_number":205,"context_line":"                # We shouldn\u0027t have more than one child per process"},{"line_number":206,"context_line":"                # so keep getting the children of the first one"},{"line_number":207,"context_line":"                pid \u003d find_child_pids(pid)[0]"},{"line_number":208,"context_line":"            except IndexError:"},{"line_number":209,"context_line":"                # Last process in the tree, return it"}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_32a8af1f","line":206,"updated":"2014-12-12 10:19:02.000000000","message":"just a note: That could eventually change for rootwrap daemon in the future. AFAIK at this moment it also has max one child per daemon.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"8088047ef7e113f1e794b908a3ca03c989c14aa5","unresolved":false,"context_lines":[{"line_number":230,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":231,"context_line":"        return list()"},{"line_number":232,"context_line":"    with open(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":233,"context_line":"        return f.readline().split(\u0027\\0\u0027)[:-1]"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":""},{"line_number":236,"context_line":"def cmdlines_are_equal(cmd1, cmd2):"}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_522a9b90","line":233,"updated":"2014-12-12 10:19:02.000000000","message":"This could go on a separate patch, or in this one, but we could remove duplication from here, making external process to call this function you made.\n\nhttps://github.com/openstack/neutron/blob/master/neutron/agent/linux/external_process.py#L123","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"737f937d14b97fddc10b7565106d4e7fcf902742","unresolved":false,"context_lines":[{"line_number":230,"context_line":"    if pid is None or not os.path.exists(\u0027/proc/%s\u0027 % pid):"},{"line_number":231,"context_line":"        return list()"},{"line_number":232,"context_line":"    with open(\u0027/proc/%s/cmdline\u0027 % pid, \u0027r\u0027) as f:"},{"line_number":233,"context_line":"        return f.readline().split(\u0027\\0\u0027)[:-1]"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":""},{"line_number":236,"context_line":"def cmdlines_are_equal(cmd1, cmd2):"}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_9a3335f5","line":233,"in_reply_to":"3a961159_522a9b90","updated":"2014-12-12 13:40:17.000000000","message":"I\u0027ll put it in my todo list as changing of external_process isn\u0027t subject of this patch.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"323d45a7ceb1fed7f8c65448e1e203792f1d2444","unresolved":false,"context_lines":[{"line_number":244,"context_line":"    return cmd1 \u003d\u003d cmd2"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":""},{"line_number":247,"context_line":"def pid_invoked_with_cmdline(pid, expected_cmd):"},{"line_number":248,"context_line":"    \"\"\"Validate process with given pid is running with provided parameters"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":25,"id":"3a961159_76aa6072","line":247,"updated":"2014-12-18 23:36:39.000000000","message":"Why would we want this method and those it depends on as part of the utils module when they\u0027re only used in functional testing?","commit_id":"4e4a4a2446a8ce36ace6e07d6cf300bdd3de3fcd"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"3be0f64663690557b89712ea869d84a0e895cb20","unresolved":false,"context_lines":[{"line_number":244,"context_line":"    return cmd1 \u003d\u003d cmd2"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":""},{"line_number":247,"context_line":"def pid_invoked_with_cmdline(pid, expected_cmd):"},{"line_number":248,"context_line":"    \"\"\"Validate process with given pid is running with provided parameters"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":"    \"\"\""}],"source_content_type":"text/x-python","patch_set":25,"id":"3a961159_cc8f3275","line":247,"in_reply_to":"3a961159_76aa6072","updated":"2015-01-02 13:08:00.000000000","message":"Done","commit_id":"4e4a4a2446a8ce36ace6e07d6cf300bdd3de3fcd"}],"neutron/tests/functional/agent/linux/helpers.py":[{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":92,"context_line":"            utils.execute([\u0027kill\u0027, \u0027-9\u0027, pid],"},{"line_number":93,"context_line":"                          root_helper\u003dself.root_helper)"},{"line_number":94,"context_line":"        else:"},{"line_number":95,"context_line":"            LOG.warning(_LW(\u0027Trying to stop process %(cmd)s with pid %(pid)d \u0027"},{"line_number":96,"context_line":"                            \u0027that is no longer running.\u0027), {\u0027cmd\u0027: self.cmd,"},{"line_number":97,"context_line":"                                                            \u0027pid\u0027: self.pid})"},{"line_number":98,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_c6451444","line":95,"updated":"2014-10-15 11:31:16.000000000","message":"Since this is a test, I wonder how visible this log will be? Is the logging mechanism initialised in this test?","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":92,"context_line":"            utils.execute([\u0027kill\u0027, \u0027-9\u0027, pid],"},{"line_number":93,"context_line":"                          root_helper\u003dself.root_helper)"},{"line_number":94,"context_line":"        else:"},{"line_number":95,"context_line":"            LOG.warning(_LW(\u0027Trying to stop process %(cmd)s with pid %(pid)d \u0027"},{"line_number":96,"context_line":"                            \u0027that is no longer running.\u0027), {\u0027cmd\u0027: self.cmd,"},{"line_number":97,"context_line":"                                                            \u0027pid\u0027: self.pid})"},{"line_number":98,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_47e03efa","line":95,"in_reply_to":"baa201ad_c6451444","updated":"2014-10-15 12:53:33.000000000","message":"Yes, in case test running with testr fails, logs are printed.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        return stream.readline()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def write(self, data):"},{"line_number":115,"context_line":"        self.stdin.write(data + \u0027\\n\u0027)"},{"line_number":116,"context_line":"        self.stdin.flush()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_66de286f","line":115,"updated":"2014-10-15 11:31:16.000000000","message":"nit: change \u0027\\n\u0027 to os.linesep","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        return stream.readline()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"    def write(self, data):"},{"line_number":115,"context_line":"        self.stdin.write(data + \u0027\\n\u0027)"},{"line_number":116,"context_line":"        self.stdin.flush()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_a7c56a5d","line":115,"in_reply_to":"baa201ad_66de286f","updated":"2014-10-15 12:53:33.000000000","message":"Done","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":116,"context_line":"        self.stdin.flush()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":119,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        with eventlet.timeout.Timeout(timeout,"},{"line_number":122,"context_line":"                RuntimeError(\"Process %s hasn\u0027t been spawned in %d seconds\" %"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_260d00e9","line":119,"updated":"2014-10-15 11:31:16.000000000","message":"Why not use the Waiter class in this function?","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"5ddea35d83f6ac6252b43fe9c80e6e996829926e","unresolved":false,"context_lines":[{"line_number":116,"context_line":"        self.stdin.flush()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":119,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        with eventlet.timeout.Timeout(timeout,"},{"line_number":122,"context_line":"                RuntimeError(\"Process %s hasn\u0027t been spawned in %d seconds\" %"}],"source_content_type":"text/x-python","patch_set":8,"id":"9aa7fdbe_2f1aaa17","line":119,"in_reply_to":"9aa7fdbe_7b5aca84","updated":"2014-10-20 15:45:18.000000000","message":"Done","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"c353af4c869e299f644656782818bb8ff84b2c8d","unresolved":false,"context_lines":[{"line_number":116,"context_line":"        self.stdin.flush()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":119,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        with eventlet.timeout.Timeout(timeout,"},{"line_number":122,"context_line":"                RuntimeError(\"Process %s hasn\u0027t been spawned in %d seconds\" %"}],"source_content_type":"text/x-python","patch_set":8,"id":"9aa7fdbe_7b5aca84","line":119,"in_reply_to":"baa201ad_07a8f68b","updated":"2014-10-19 10:58:20.000000000","message":"The predicate could be a function in this class which accepts \u0027self\u0027 and changes self.child_pid. A bit ugly but might be better than the code duplication there exists now.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":116,"context_line":"        self.stdin.flush()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":119,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        with eventlet.timeout.Timeout(timeout,"},{"line_number":122,"context_line":"                RuntimeError(\"Process %s hasn\u0027t been spawned in %d seconds\" %"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_07a8f68b","line":119,"in_reply_to":"baa201ad_260d00e9","updated":"2014-10-15 12:53:33.000000000","message":"There is no predicate as we\u0027re interested in the output of get_child_pid.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"316280fd19bfa8a0f8e9f6d2c69df540cceb74f7","unresolved":false,"context_lines":[{"line_number":128,"context_line":"                    pid, root_helper\u003dself.root_helper)"},{"line_number":129,"context_line":"                eventlet.sleep(sleep)"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def wait_for_death(self, timeout\u003d5, sleep\u003d1):"},{"line_number":132,"context_line":"        not_exist \u003d (lambda proc, path:"},{"line_number":133,"context_line":"                     proc.poll() is not None and not os.path.exists(path))"},{"line_number":134,"context_line":"        waiter \u003d Waiter(timeout, sleep)"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_663b88d1","line":131,"updated":"2014-10-15 11:31:16.000000000","message":"Function name sounds funny ^_^\n\nWhy not just use Popen.wait()? eventlet overrides Popen\u0027s wait so that it\u0027s safe, and you can also pass it timeout and check_interval parameters so it will function exactly the same.","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6642fc09722bea67c1f68a5a9e7d3f7f2a16723d","unresolved":false,"context_lines":[{"line_number":128,"context_line":"                    pid, root_helper\u003dself.root_helper)"},{"line_number":129,"context_line":"                eventlet.sleep(sleep)"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"    def wait_for_death(self, timeout\u003d5, sleep\u003d1):"},{"line_number":132,"context_line":"        not_exist \u003d (lambda proc, path:"},{"line_number":133,"context_line":"                     proc.poll() is not None and not os.path.exists(path))"},{"line_number":134,"context_line":"        waiter \u003d Waiter(timeout, sleep)"}],"source_content_type":"text/x-python","patch_set":8,"id":"baa201ad_67d2a201","line":131,"in_reply_to":"baa201ad_663b88d1","updated":"2014-10-15 12:53:33.000000000","message":"If you look here https://review.openstack.org/#/c/120349/12..13/neutron/tests/functional/agent/linux/helpers.py I also tried it with wait(). I encountered problems with destroying the namespace in the cleanup because the device was busy. Then again, maybe it was ubuntu bug, I just realized I haven\u0027t tried wait() on Ubuntu 14. Thanks for point it out ;)","commit_id":"eb1332296c93d3c95316e80e139e3a22392794e9"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":21,"context_line":"from neutron.agent.linux import utils"},{"line_number":22,"context_line":"from neutron.openstack.common import log as logging"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"CHILD_PROCESS_TIMEOUT \u003d 5"},{"line_number":27,"context_line":"CHILD_PROCESS_SLEEP \u003d 0.5"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_03f2796f","line":24,"updated":"2014-10-27 17:17:20.000000000","message":"This doesn\u0027t appear to be used.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":21,"context_line":"from neutron.agent.linux import utils"},{"line_number":22,"context_line":"from neutron.openstack.common import log as logging"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"CHILD_PROCESS_TIMEOUT \u003d 5"},{"line_number":27,"context_line":"CHILD_PROCESS_SLEEP \u003d 0.5"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_88261d15","line":24,"in_reply_to":"9aa7fdbe_03f2796f","updated":"2014-11-11 17:05:34.000000000","message":"Good eye! Thanks.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"CHILD_PROCESS_TIMEOUT \u003d 5"},{"line_number":27,"context_line":"CHILD_PROCESS_SLEEP \u003d 0.5"},{"line_number":28,"context_line":"READ_TIMEOUT \u003d 2"},{"line_number":29,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_9dbf9960","line":26,"updated":"2014-10-27 17:17:20.000000000","message":"These seem pretty arbitrary - don\u0027t we risk running into timing issues on boxes with more load than you\u0027ve been testing with?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"b8a47641ee4dd3de9ac3722a5dcd7f8cb8edf235","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"CHILD_PROCESS_TIMEOUT \u003d 5"},{"line_number":27,"context_line":"CHILD_PROCESS_SLEEP \u003d 0.5"},{"line_number":28,"context_line":"READ_TIMEOUT \u003d 2"},{"line_number":29,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_d4d192fc","line":26,"in_reply_to":"5a890539_48b515ed","updated":"2014-11-12 08:33:38.000000000","message":"The gate is even more high-loaded. I\u0027d go with something like 30/60 seconds, and even then there are risks of nondeterministic errors.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":23,"context_line":""},{"line_number":24,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"CHILD_PROCESS_TIMEOUT \u003d 5"},{"line_number":27,"context_line":"CHILD_PROCESS_SLEEP \u003d 0.5"},{"line_number":28,"context_line":"READ_TIMEOUT \u003d 2"},{"line_number":29,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_48b515ed","line":26,"in_reply_to":"9aa7fdbe_9dbf9960","updated":"2014-11-11 17:05:34.000000000","message":"Maybe, 5 seconds seem to be quite high even for high loaded machine. What do you suggest?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":68,"context_line":"            pass"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"class Process(subprocess.Popen):"},{"line_number":72,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":73,"context_line":"        # change defaults from Popen"},{"line_number":74,"context_line":"        for arg in (\u0027stdin\u0027, \u0027stdout\u0027, \u0027stderr\u0027):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_a0db637a","line":71,"updated":"2014-10-27 17:17:20.000000000","message":"The name of this class could be more explicit, given that it\u0027s utility is in knowing how to manage rootwrap-invoked processes.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":68,"context_line":"            pass"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"class Process(subprocess.Popen):"},{"line_number":72,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":73,"context_line":"        # change defaults from Popen"},{"line_number":74,"context_line":"        for arg in (\u0027stdin\u0027, \u0027stdout\u0027, \u0027stderr\u0027):"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_a8d839f0","line":71,"in_reply_to":"9aa7fdbe_a0db637a","updated":"2014-11-11 17:05:34.000000000","message":"RoothelperProcess or NamespaceProcess or RoothelperNamespaceProcess? I don\u0027t know, would docstring help? I\u0027m not a fan of long class java-style enterprise names and nothing good comes into my mind.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":70,"context_line":""},{"line_number":71,"context_line":"class Process(subprocess.Popen):"},{"line_number":72,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":73,"context_line":"        # change defaults from Popen"},{"line_number":74,"context_line":"        for arg in (\u0027stdin\u0027, \u0027stdout\u0027, \u0027stderr\u0027):"},{"line_number":75,"context_line":"            kwargs[arg] \u003d kwargs.get(arg, subprocess.PIPE)"},{"line_number":76,"context_line":"        self.namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_e0456b43","line":73,"updated":"2014-10-27 17:17:20.000000000","message":"Please use dict.setdefault() instead: https://docs.python.org/2/library/stdtypes.html#dict.setdefault\n\nThis comment will then become unnecessary as the intent will be obvious.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":70,"context_line":""},{"line_number":71,"context_line":"class Process(subprocess.Popen):"},{"line_number":72,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":73,"context_line":"        # change defaults from Popen"},{"line_number":74,"context_line":"        for arg in (\u0027stdin\u0027, \u0027stdout\u0027, \u0027stderr\u0027):"},{"line_number":75,"context_line":"            kwargs[arg] \u003d kwargs.get(arg, subprocess.PIPE)"},{"line_number":76,"context_line":"        self.namespace \u003d kwargs.pop(\u0027namespace\u0027, None)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_68881120","line":73,"in_reply_to":"9aa7fdbe_e0456b43","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":77,"context_line":"        self.root_helper \u003d kwargs.pop(\u0027root_helper\u0027, None)"},{"line_number":78,"context_line":"        self.cmd \u003d cmd"},{"line_number":79,"context_line":"        if self.namespace is not None:"},{"line_number":80,"context_line":"            cmd \u003d [\u0027ip\u0027, \u0027netns\u0027, \u0027exec\u0027, self.namespace] + cmd"},{"line_number":81,"context_line":"        if self.root_helper is not None:"},{"line_number":82,"context_line":"            cmd \u003d shlex.split(self.root_helper) + cmd"},{"line_number":83,"context_line":"        self.child_pid \u003d None"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_210df537","line":80,"updated":"2014-10-27 17:17:20.000000000","message":"Is there a reason not to use iplib to compose the commands here?  It already handles the case of handling root helper inclusion, too.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":77,"context_line":"        self.root_helper \u003d kwargs.pop(\u0027root_helper\u0027, None)"},{"line_number":78,"context_line":"        self.cmd \u003d cmd"},{"line_number":79,"context_line":"        if self.namespace is not None:"},{"line_number":80,"context_line":"            cmd \u003d [\u0027ip\u0027, \u0027netns\u0027, \u0027exec\u0027, self.namespace] + cmd"},{"line_number":81,"context_line":"        if self.root_helper is not None:"},{"line_number":82,"context_line":"            cmd \u003d shlex.split(self.root_helper) + cmd"},{"line_number":83,"context_line":"        self.child_pid \u003d None"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_a8f59984","line":80,"in_reply_to":"9aa7fdbe_210df537","updated":"2014-11-11 17:05:34.000000000","message":"iplib encapsulates command execution and returns only strings. We care here about the Popen object.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":102,"context_line":"            poller \u003d select.poll()"},{"line_number":103,"context_line":"            poller.register(stream.fileno())"},{"line_number":104,"context_line":"            waiter.wait_until(poller.poll, 1)"},{"line_number":105,"context_line":"            return stream.readline()"},{"line_number":106,"context_line":"        return stream.readline()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    def write(self, data):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_a011a353","line":105,"updated":"2014-10-27 17:17:20.000000000","message":"This line is duplicated by the one that follows and should be removed.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":102,"context_line":"            poller \u003d select.poll()"},{"line_number":103,"context_line":"            poller.register(stream.fileno())"},{"line_number":104,"context_line":"            waiter.wait_until(poller.poll, 1)"},{"line_number":105,"context_line":"            return stream.readline()"},{"line_number":106,"context_line":"        return stream.readline()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    def write(self, data):"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_08894d26","line":105,"in_reply_to":"9aa7fdbe_a011a353","updated":"2014-11-11 17:05:34.000000000","message":"Oops, Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":105,"context_line":"            return stream.readline()"},{"line_number":106,"context_line":"        return stream.readline()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    def write(self, data):"},{"line_number":109,"context_line":"        self.stdin.write(data + os.linesep)"},{"line_number":110,"context_line":"        self.stdin.flush()"},{"line_number":111,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_202db395","line":108,"updated":"2014-10-27 17:17:20.000000000","message":"Wouldn\u0027t this more properly be called writeline, since a line seperator is automatically added?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":105,"context_line":"            return stream.readline()"},{"line_number":106,"context_line":"        return stream.readline()"},{"line_number":107,"context_line":""},{"line_number":108,"context_line":"    def write(self, data):"},{"line_number":109,"context_line":"        self.stdin.write(data + os.linesep)"},{"line_number":110,"context_line":"        self.stdin.flush()"},{"line_number":111,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_e8f9614e","line":108,"in_reply_to":"9aa7fdbe_202db395","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":113,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":114,"context_line":"        def child_setter():"},{"line_number":115,"context_line":"            retval \u003d utils.check_pid_with_cmd(self.child_pid, self.cmd)"},{"line_number":116,"context_line":"            self.child_pid \u003d utils.get_child_pid("},{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_1d572959","line":114,"updated":"2014-10-27 17:17:20.000000000","message":"This name doesn\u0027t make much sense.  Something like \u0027is_child_alive\u0027 would seem to make more sense.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"fa7fd727b67c1247ad1abeebd4610f4d0bdaf205","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":113,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":114,"context_line":"        def child_setter():"},{"line_number":115,"context_line":"            retval \u003d utils.check_pid_with_cmd(self.child_pid, self.cmd)"},{"line_number":116,"context_line":"            self.child_pid \u003d utils.get_child_pid("},{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_c137444c","line":114,"in_reply_to":"5a890539_3fed9825","updated":"2014-12-05 14:06:57.000000000","message":"I don\u0027t understand your idea. You just rewrote 4 lines below with different name functions. Do you suggest to keep behavior in this patchset rather than patchset 20?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"cdae40eea1d8ce7b17c66107fd1640f5fdd7861d","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":113,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":114,"context_line":"        def child_setter():"},{"line_number":115,"context_line":"            retval \u003d utils.check_pid_with_cmd(self.child_pid, self.cmd)"},{"line_number":116,"context_line":"            self.child_pid \u003d utils.get_child_pid("},{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_3fed9825","line":114,"in_reply_to":"5a890539_74645e49","updated":"2014-12-05 00:56:50.000000000","message":"IMO:  \u0027self.child_pid \u003d wait_until(is_child_alive)\u0027 doesn\u0027t read well.  Why would I expect \"wait_until\" to return anything?  I would leave it up to the predicate to do this instead of building a strange behavior in to the common helper:\n\n  def record_child_pid_if_alive():\n      pid \u003d is_child_alive()\n      if pid:\n          self.child_pid \u003d pid\n          return True\n\n  wait_until(record_child_pid_if_alive)\n\nThis is still an odd behavior and I would personally look to redesign it but at least I didn\u0027t taint the wait_until helper with a non-obvious behavior.  At least, I would probably rename is_child_alive to get_child_pid_if_alive.\n\nMy $0.02","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"7e6aa414182b174ba3e3533f47a33e349232a932","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":113,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":114,"context_line":"        def child_setter():"},{"line_number":115,"context_line":"            retval \u003d utils.check_pid_with_cmd(self.child_pid, self.cmd)"},{"line_number":116,"context_line":"            self.child_pid \u003d utils.get_child_pid("},{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_a9adb286","line":114,"in_reply_to":"5a890539_c137444c","updated":"2014-12-05 18:57:54.000000000","message":"My code snippet was intended to be equivalent to this:  \"self.child_pid \u003d wait_until(is_child_alive)\" which you wrote in your comment.  It does look similar to the code that you already had in this patch set.  I guess what I\u0027m saying is that I\u0027d prefer to be a little more verbose here than to count on a somewhat surprising behavior of wait_until.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"b8a47641ee4dd3de9ac3722a5dcd7f8cb8edf235","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":113,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":114,"context_line":"        def child_setter():"},{"line_number":115,"context_line":"            retval \u003d utils.check_pid_with_cmd(self.child_pid, self.cmd)"},{"line_number":116,"context_line":"            self.child_pid \u003d utils.get_child_pid("},{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_74645e49","line":114,"in_reply_to":"5a890539_c8066520","updated":"2014-11-12 08:33:38.000000000","message":"What about:\n\n1. Changing wait_until to return the value of the predicate. This is reasonable since the code there ([1]) checks \u0027while not predicate\u0027, ie. checks if the predicate returned non-False and non-None (and non-0, etc...0), but the last predicate\u0027s return value might be of use.\n\n2. Change this code to simply do \u0027self.child_pid \u003d wait_until(is_child_alive)\u0027\n\n[1]: https://review.openstack.org/#/c/125109/8/neutron/tests/functional/agent/linux/helpers.py","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"f80561bc776bc5e9853f13cd653918ca320cb9f9","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":113,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":114,"context_line":"        def child_setter():"},{"line_number":115,"context_line":"            retval \u003d utils.check_pid_with_cmd(self.child_pid, self.cmd)"},{"line_number":116,"context_line":"            self.child_pid \u003d utils.get_child_pid("},{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_e211d404","line":114,"in_reply_to":"9aa7fdbe_1d572959","updated":"2014-10-28 07:54:19.000000000","message":"I disagree, since the function actually waits until such a time when the child is no longer alive (or raises a Timeout error).\n\nAnyway, the \u0027wait\u0027 part of the function name corresponds nicely to the syscall \u0027wait\u0027, so I actually really like the name of the function :)","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":113,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":114,"context_line":"        def child_setter():"},{"line_number":115,"context_line":"            retval \u003d utils.check_pid_with_cmd(self.child_pid, self.cmd)"},{"line_number":116,"context_line":"            self.child_pid \u003d utils.get_child_pid("},{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_c8066520","line":114,"in_reply_to":"9aa7fdbe_68885971","updated":"2014-11-11 17:05:34.000000000","message":"is_child_alive seems like checking if child is alive. We\u0027re more interesting here in setting correct self.child_pid while waiting for the correct process (see patchset 8).\nWhat about \"check_and_set_child_process\"?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"acc88daf5358a336da07d97abeeca4cc7bb0ffab","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"    def _wait_for_child_process(self, timeout\u003dCHILD_PROCESS_TIMEOUT,"},{"line_number":113,"context_line":"                                sleep\u003dCHILD_PROCESS_SLEEP):"},{"line_number":114,"context_line":"        def child_setter():"},{"line_number":115,"context_line":"            retval \u003d utils.check_pid_with_cmd(self.child_pid, self.cmd)"},{"line_number":116,"context_line":"            self.child_pid \u003d utils.get_child_pid("},{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_68885971","line":114,"in_reply_to":"9aa7fdbe_e211d404","updated":"2014-10-28 14:49:22.000000000","message":"I wasn\u0027t talking about the name of the enclosing function (_wait_for_child_process).  As per the line that we are commenting on, I don\u0027t think \u0027child_setter\u0027 is a descriptive enough.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"066e55145548838eebd78be566bc6c68935efe02","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"},{"line_number":118,"context_line":"            return retval"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        self.child_pid \u003d utils.get_child_pid("},{"line_number":122,"context_line":"            pid, root_helper\u003dself.root_helper)"},{"line_number":123,"context_line":"        waiter \u003d Waiter(timeout,"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_1c9e9407","line":120,"updated":"2014-10-23 15:38:21.000000000","message":"nit: why not pass \u0027str(self.pid)\u0027 to get_child_pid (and remove the localized variable \u0027pid\u0027 you create in the line above)?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0ee93bd75e8f27c96ea660d8af550c7e482a0781","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"},{"line_number":118,"context_line":"            return retval"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        self.child_pid \u003d utils.get_child_pid("},{"line_number":122,"context_line":"            pid, root_helper\u003dself.root_helper)"},{"line_number":123,"context_line":"        waiter \u003d Waiter(timeout,"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_2dec048f","line":120,"in_reply_to":"9aa7fdbe_1c9e9407","updated":"2014-10-23 16:52:10.000000000","message":"It\u0027s also used on L117 from closure","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"79e7b002885be3ae39dc7cb0e00a2387632bc514","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"},{"line_number":118,"context_line":"            return retval"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        self.child_pid \u003d utils.get_child_pid("},{"line_number":122,"context_line":"            pid, root_helper\u003dself.root_helper)"},{"line_number":123,"context_line":"        waiter \u003d Waiter(timeout,"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_8f176344","line":120,"in_reply_to":"9aa7fdbe_2dec048f","updated":"2014-10-27 15:53:28.000000000","message":"I didn\u0027t see that, sorry.\n\nAnyway, it\u0027s a bit ugly to have child_setter reference something which isn\u0027t defined yet... maybe L127 can be amended to also send the variable to the function (and change child_setter to receive pid as parameter)?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"},{"line_number":118,"context_line":"            return retval"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        self.child_pid \u003d utils.get_child_pid("},{"line_number":122,"context_line":"            pid, root_helper\u003dself.root_helper)"},{"line_number":123,"context_line":"        waiter \u003d Waiter(timeout,"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_dd404109","line":120,"in_reply_to":"9aa7fdbe_8f176344","updated":"2014-10-27 17:17:20.000000000","message":"+1 to defining child_setter after the things it references.  But why do we need to transform self.pid?  Why couldn\u0027t child_setter reference self.pid?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                pid, root_helper\u003dself.root_helper)"},{"line_number":118,"context_line":"            return retval"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"        pid \u003d str(self.pid)"},{"line_number":121,"context_line":"        self.child_pid \u003d utils.get_child_pid("},{"line_number":122,"context_line":"            pid, root_helper\u003dself.root_helper)"},{"line_number":123,"context_line":"        waiter \u003d Waiter(timeout,"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_1366e251","line":120,"in_reply_to":"9aa7fdbe_dd404109","updated":"2014-11-11 17:05:34.000000000","message":"Maybe we can, I\u0027ll test that and type pid to str in get_child_pid().","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"3d41816aa18a138118371b0bd2f2c4ce0d57e9e1","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"from neutron.agent.linux import utils"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"CHILD_PROCESS_TIMEOUT \u003d os.environ.get(\u0027OS_TEST_CHILD_PROCESS_TIMEOUT\u0027, 5)"},{"line_number":25,"context_line":"CHILD_PROCESS_SLEEP \u003d os.environ.get(\u0027OS_TEST_CHILD_PROCESS_SLEEP\u0027, 0.5)"},{"line_number":26,"context_line":"READ_TIMEOUT \u003d os.environ.get(\u0027OS_TEST_READ_TIMEOUT\u0027, 2)"},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_ac78d4c4","line":24,"updated":"2014-12-12 00:41:27.000000000","message":"As per comments on PS12, I\u0027m concerned that we\u0027re going to run into timing issues in the future and that it may make sense to increase the timeouts to much larger values here.  Either that, or update tox.ini\u0027s entry for dsvm-functional to ensure more reasonable timeouts for use by the gate.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"737f937d14b97fddc10b7565106d4e7fcf902742","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"from neutron.agent.linux import utils"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"CHILD_PROCESS_TIMEOUT \u003d os.environ.get(\u0027OS_TEST_CHILD_PROCESS_TIMEOUT\u0027, 5)"},{"line_number":25,"context_line":"CHILD_PROCESS_SLEEP \u003d os.environ.get(\u0027OS_TEST_CHILD_PROCESS_SLEEP\u0027, 0.5)"},{"line_number":26,"context_line":"READ_TIMEOUT \u003d os.environ.get(\u0027OS_TEST_READ_TIMEOUT\u0027, 2)"},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_ba5c711a","line":24,"in_reply_to":"3a961159_ac78d4c4","updated":"2014-12-12 13:40:17.000000000","message":"Any suggestion for specific values to avoid conversation about values in upcoming patch?","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"8088047ef7e113f1e794b908a3ca03c989c14aa5","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"from neutron.agent.linux import utils"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"CHILD_PROCESS_TIMEOUT \u003d os.environ.get(\u0027OS_TEST_CHILD_PROCESS_TIMEOUT\u0027, 5)"},{"line_number":25,"context_line":"CHILD_PROCESS_SLEEP \u003d os.environ.get(\u0027OS_TEST_CHILD_PROCESS_SLEEP\u0027, 0.5)"},{"line_number":26,"context_line":"READ_TIMEOUT \u003d os.environ.get(\u0027OS_TEST_READ_TIMEOUT\u0027, 2)"},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_92f5e358","line":24,"in_reply_to":"3a961159_ac78d4c4","updated":"2014-12-12 10:19:02.000000000","message":"Total agreement here.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"78ba0d9f4168a61f3d5768e66eb0916bcd2758bb","unresolved":false,"context_lines":[{"line_number":21,"context_line":""},{"line_number":22,"context_line":"from neutron.agent.linux import utils"},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"CHILD_PROCESS_TIMEOUT \u003d os.environ.get(\u0027OS_TEST_CHILD_PROCESS_TIMEOUT\u0027, 5)"},{"line_number":25,"context_line":"CHILD_PROCESS_SLEEP \u003d os.environ.get(\u0027OS_TEST_CHILD_PROCESS_SLEEP\u0027, 0.5)"},{"line_number":26,"context_line":"READ_TIMEOUT \u003d os.environ.get(\u0027OS_TEST_READ_TIMEOUT\u0027, 2)"},{"line_number":27,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_2eb6838a","line":24,"in_reply_to":"3a961159_ba5c711a","updated":"2014-12-12 15:50:49.000000000","message":"As per John\u0027s suggestion, I think 20s is appropriate for a timeout that could be impacted by parallel execution.  We can always adjust upwards/downwards from there if it causes trouble.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8124,"name":"cbrandily","email":"zzelle@gmail.com","username":"cbrandily"},"change_message_id":"5c02eb96f4b21805d426eb33adcfb91d23626c9c","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            pass"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"class RootHelperProcess(subprocess.Popen):"},{"line_number":77,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":78,"context_line":"        for arg in (\u0027stdin\u0027, \u0027stdout\u0027, \u0027stderr\u0027):"},{"line_number":79,"context_line":"            kwargs.setdefault(arg, subprocess.PIPE)"}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_94f14c97","line":76,"updated":"2014-12-12 13:02:28.000000000","message":"Perhaps i miss something but why such helper is not in neutron.agent.linux.utils? A docstring would be great","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"737f937d14b97fddc10b7565106d4e7fcf902742","unresolved":false,"context_lines":[{"line_number":73,"context_line":"            pass"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"class RootHelperProcess(subprocess.Popen):"},{"line_number":77,"context_line":"    def __init__(self, cmd, *args, **kwargs):"},{"line_number":78,"context_line":"        for arg in (\u0027stdin\u0027, \u0027stdout\u0027, \u0027stderr\u0027):"},{"line_number":79,"context_line":"            kwargs.setdefault(arg, subprocess.PIPE)"}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_ba455144","line":76,"in_reply_to":"3a961159_94f14c97","updated":"2014-12-12 13:40:17.000000000","message":"For monitoring external processes we have more powerful tools that encapsulates Popen object. For testing purposes I need to handle I/O of spawned process.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"5c7ae5c4175ee52894139f1b172bf9be8ebbb9d1","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            if utils.pid_invoked_with_cmdline(child_pid, self.cmd):"},{"line_number":121,"context_line":"                return child_pid"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        self.child_pid \u003d wait_until_true(is_child_spawned,"},{"line_number":124,"context_line":"                                         timeout,"},{"line_number":125,"context_line":"                                         exception\u003dRuntimeError("},{"line_number":126,"context_line":"                                             \"Process %s hasn\u0027t been spawned \""}],"source_content_type":"text/x-python","patch_set":20,"id":"5a890539_ffb5d018","line":123,"updated":"2014-12-05 00:57:30.000000000","message":"See PS12 comment.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"8088047ef7e113f1e794b908a3ca03c989c14aa5","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            if utils.pid_invoked_with_cmdline(child_pid, self.cmd):"},{"line_number":121,"context_line":"                return child_pid"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        self.child_pid \u003d wait_until_true(is_child_spawned,"},{"line_number":124,"context_line":"                                         timeout,"},{"line_number":125,"context_line":"                                         exception\u003dRuntimeError("},{"line_number":126,"context_line":"                                             \"Process %s hasn\u0027t been spawned \""}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_8d54e8ff","line":123,"in_reply_to":"3a961159_4c321060","updated":"2014-12-12 10:19:02.000000000","message":"Yes, also, I wouldn\u0027t expect a pid from \"is_child_spawned\"\n\nMay be everything becomes more clear if \"wait_until_true\" does not return anything (it will exit when True, so you asume it\u0027s true, an not !\u003dFalse)\n\nAnd then is_child_spawned returns True / False\n\nand you have another function get_child_pid which returns pid, and you can call this function from is_child_spawned.","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"737f937d14b97fddc10b7565106d4e7fcf902742","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            if utils.pid_invoked_with_cmdline(child_pid, self.cmd):"},{"line_number":121,"context_line":"                return child_pid"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        self.child_pid \u003d wait_until_true(is_child_spawned,"},{"line_number":124,"context_line":"                                         timeout,"},{"line_number":125,"context_line":"                                         exception\u003dRuntimeError("},{"line_number":126,"context_line":"                                             \"Process %s hasn\u0027t been spawned \""}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_1af3c50a","line":123,"in_reply_to":"3a961159_8d54e8ff","updated":"2014-12-12 13:40:17.000000000","message":"Done","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"3d41816aa18a138118371b0bd2f2c4ce0d57e9e1","unresolved":false,"context_lines":[{"line_number":120,"context_line":"            if utils.pid_invoked_with_cmdline(child_pid, self.cmd):"},{"line_number":121,"context_line":"                return child_pid"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":"        self.child_pid \u003d wait_until_true(is_child_spawned,"},{"line_number":124,"context_line":"                                         timeout,"},{"line_number":125,"context_line":"                                         exception\u003dRuntimeError("},{"line_number":126,"context_line":"                                             \"Process %s hasn\u0027t been spawned \""}],"source_content_type":"text/x-python","patch_set":20,"id":"3a961159_4c321060","line":123,"in_reply_to":"5a890539_ffb5d018","updated":"2014-12-12 00:41:27.000000000","message":"I agree with Carl\u0027s comment from patchset 12 that returning a result from wait_until_true is unintuitive.  Is there a reason not to follow his suggestion?","commit_id":"a292275029366944475da7e3a398f788fb06b6a3"}],"neutron/tests/functional/agent/linux/test_external_process.py":[{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"32cbac36058409bc34bdc411d26ad69b2d24faf9","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        with self.assert_max_execution_time():"},{"line_number":28,"context_line":"            proc \u003d external_process.Process("},{"line_number":29,"context_line":"                [\u0027tee\u0027], root_helper\u003dself.root_helper)"},{"line_number":30,"context_line":"            # Write to make sure child process was actually spawned"},{"line_number":31,"context_line":"            proc.wait_for_child_process()"},{"line_number":32,"context_line":"            proc.kill()"},{"line_number":33,"context_line":"            proc.wait()"}],"source_content_type":"text/x-python","patch_set":6,"id":"da9df570_fc319d5a","line":30,"updated":"2014-09-28 16:39:10.000000000","message":"Since wait_for_child_process is used only in testing it should probably be moved to test code. I know you only added the Process class for testing but it itself isn\u0027t defined in a test directory / file.","commit_id":"8ee58af4b91c0ced5dee0509712bc39b893d482d"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"729ebf1089d5ecdfdde6437dfa5dac921af59a4b","unresolved":false,"context_lines":[{"line_number":27,"context_line":"        with self.assert_max_execution_time():"},{"line_number":28,"context_line":"            proc \u003d external_process.Process("},{"line_number":29,"context_line":"                [\u0027tee\u0027], root_helper\u003dself.root_helper)"},{"line_number":30,"context_line":"            # Write to make sure child process was actually spawned"},{"line_number":31,"context_line":"            proc.wait_for_child_process()"},{"line_number":32,"context_line":"            proc.kill()"},{"line_number":33,"context_line":"            proc.wait()"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_be63e5da","line":30,"in_reply_to":"da9df570_fc319d5a","updated":"2014-10-02 14:07:44.000000000","message":"I\u0027ll move the Process to testing directory as it\u0027s basically only a helper for functional test.","commit_id":"8ee58af4b91c0ced5dee0509712bc39b893d482d"}],"neutron/tests/functional/agent/linux/test_helpers.py":[{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":28,"context_line":"        with self.assert_max_execution_time():"},{"line_number":29,"context_line":"            proc \u003d helpers.Process("},{"line_number":30,"context_line":"                [\u0027tee\u0027], root_helper\u003dself.root_helper)"},{"line_number":31,"context_line":"            # Write to make sure child process was actually spawned"},{"line_number":32,"context_line":"            proc.kill()"},{"line_number":33,"context_line":"            proc.wait()"},{"line_number":34,"context_line":"            # sudo returns 137 and"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_80757f65","line":31,"updated":"2014-10-27 17:17:20.000000000","message":"This comment doesn\u0027t appear to match the code.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":28,"context_line":"        with self.assert_max_execution_time():"},{"line_number":29,"context_line":"            proc \u003d helpers.Process("},{"line_number":30,"context_line":"                [\u0027tee\u0027], root_helper\u003dself.root_helper)"},{"line_number":31,"context_line":"            # Write to make sure child process was actually spawned"},{"line_number":32,"context_line":"            proc.kill()"},{"line_number":33,"context_line":"            proc.wait()"},{"line_number":34,"context_line":"            # sudo returns 137 and"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_2ed291d2","line":31,"in_reply_to":"9aa7fdbe_80757f65","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"fd0df208fafeb8a159caaf3836fd2aff8d13867e","unresolved":false,"context_lines":[{"line_number":16,"context_line":"from neutron.tests.functional.agent.linux import helpers"},{"line_number":17,"context_line":""},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"class TestRoothelperProcess(base.BaseLinuxTestCase):"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def test_process_read_write(self):"},{"line_number":22,"context_line":"        proc \u003d helpers.RoothelperProcess([\u0027tee\u0027], root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_b26bb11c","line":19,"updated":"2014-11-17 12:33:14.000000000","message":"nit: RootHelper","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"b77bec89c9b59d1de62e1f253b662ee0e263612d","unresolved":false,"context_lines":[{"line_number":16,"context_line":"from neutron.tests.functional.agent.linux import helpers"},{"line_number":17,"context_line":""},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"class TestRoothelperProcess(base.BaseLinuxTestCase):"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def test_process_read_write(self):"},{"line_number":22,"context_line":"        proc \u003d helpers.RoothelperProcess([\u0027tee\u0027], root_helper\u003dself.root_helper)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_303d9dbc","line":19,"in_reply_to":"5a890539_b26bb11c","updated":"2014-11-19 16:23:58.000000000","message":"Done","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"},{"author":{"_account_id":12444,"name":"John Schwarz","email":"jschwarz@redhat.com","username":"jschwarz"},"change_message_id":"fd0df208fafeb8a159caaf3836fd2aff8d13867e","unresolved":false,"context_lines":[{"line_number":19,"context_line":"class TestRoothelperProcess(base.BaseLinuxTestCase):"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def test_process_read_write(self):"},{"line_number":22,"context_line":"        proc \u003d helpers.RoothelperProcess([\u0027tee\u0027], root_helper\u003dself.root_helper)"},{"line_number":23,"context_line":"        proc.writeline(\u0027foo\u0027)"},{"line_number":24,"context_line":"        output \u003d proc.read_stdout(helpers.READ_TIMEOUT)"},{"line_number":25,"context_line":"        self.assertEqual(\u0027foo\\n\u0027, output)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_d2767d05","line":22,"updated":"2014-11-17 12:33:14.000000000","message":"nit: RootHelper","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"b77bec89c9b59d1de62e1f253b662ee0e263612d","unresolved":false,"context_lines":[{"line_number":19,"context_line":"class TestRoothelperProcess(base.BaseLinuxTestCase):"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"    def test_process_read_write(self):"},{"line_number":22,"context_line":"        proc \u003d helpers.RoothelperProcess([\u0027tee\u0027], root_helper\u003dself.root_helper)"},{"line_number":23,"context_line":"        proc.writeline(\u0027foo\u0027)"},{"line_number":24,"context_line":"        output \u003d proc.read_stdout(helpers.READ_TIMEOUT)"},{"line_number":25,"context_line":"        self.assertEqual(\u0027foo\\n\u0027, output)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_7047254c","line":22,"in_reply_to":"5a890539_d2767d05","updated":"2014-11-19 16:23:58.000000000","message":"Done","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"b77bec89c9b59d1de62e1f253b662ee0e263612d","unresolved":false,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    def test_process_kill(self):"},{"line_number":28,"context_line":"        with self.assert_max_execution_time():"},{"line_number":29,"context_line":"            proc \u003d helpers.RoothelperProcess("},{"line_number":30,"context_line":"                [\u0027tee\u0027], root_helper\u003dself.root_helper)"},{"line_number":31,"context_line":"            proc.kill()"},{"line_number":32,"context_line":"            proc.wait()"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a890539_d06d39ce","line":29,"updated":"2014-11-19 16:23:58.000000000","message":"Done","commit_id":"ca80280ed7d734456bc576a14f16f30c8c710d85"}],"neutron/tests/functional/agent/linux/test_ovsdb_monitor.py":[{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"cfef5bb98f2ed93fbf745a638bc0229990c66a21","unresolved":false,"context_lines":[{"line_number":79,"context_line":"            self.monitor.respawn_interval \u003d 0"},{"line_number":80,"context_line":"            old_pid \u003d self.monitor._process.pid"},{"line_number":81,"context_line":"            output1 \u003d self.collect_initial_output()"},{"line_number":82,"context_line":"            pid \u003d utils.get_child_pid(old_pid, self.root_helper)"},{"line_number":83,"context_line":"            self.monitor._kill_process(pid)"},{"line_number":84,"context_line":"            self.monitor._reset_queues()"},{"line_number":85,"context_line":"            while (self.monitor._process.pid \u003d\u003d old_pid):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_ec3ac797","line":82,"updated":"2014-10-27 10:23:39.000000000","message":"Could make sense to have a monitor._get_pid_to_kill() which does the utils.get_child_pid(old_pid, self.root_helper) ? \n\nI haven\u0027t actually looked at that part, so please ignore me if that doesn\u0027t make much sense.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":79,"context_line":"            self.monitor.respawn_interval \u003d 0"},{"line_number":80,"context_line":"            old_pid \u003d self.monitor._process.pid"},{"line_number":81,"context_line":"            output1 \u003d self.collect_initial_output()"},{"line_number":82,"context_line":"            pid \u003d utils.get_child_pid(old_pid, self.root_helper)"},{"line_number":83,"context_line":"            self.monitor._kill_process(pid)"},{"line_number":84,"context_line":"            self.monitor._reset_queues()"},{"line_number":85,"context_line":"            while (self.monitor._process.pid \u003d\u003d old_pid):"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_6ef8197d","line":82,"in_reply_to":"9aa7fdbe_635b45f6","updated":"2014-11-11 17:05:34.000000000","message":"Is it worth to have it as it\u0027s only used here?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0a58d8822d0eecbd0e5589d170a719da541f11c5","unresolved":false,"context_lines":[{"line_number":79,"context_line":"            self.monitor.respawn_interval \u003d 0"},{"line_number":80,"context_line":"            old_pid \u003d self.monitor._process.pid"},{"line_number":81,"context_line":"            output1 \u003d self.collect_initial_output()"},{"line_number":82,"context_line":"            pid \u003d utils.get_child_pid(old_pid, self.root_helper)"},{"line_number":83,"context_line":"            self.monitor._kill_process(pid)"},{"line_number":84,"context_line":"            self.monitor._reset_queues()"},{"line_number":85,"context_line":"            while (self.monitor._process.pid \u003d\u003d old_pid):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_635b45f6","line":82,"in_reply_to":"9aa7fdbe_ec3ac797","updated":"2014-10-27 11:27:06.000000000","message":"I\u0027ll take a look, thanks for suggestion!","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"}],"neutron/tests/unit/agent/linux/test_async_process.py":[{"author":{"_account_id":13538,"name":"Claudiu Popa","email":"cpopa@cloudbasesolutions.com","username":"PCManticore"},"change_message_id":"3b086189e082e6552d6e8315924b1cb62e246c1c","unresolved":false,"context_lines":[{"line_number":155,"context_line":"        self._test_iter_output_calls_iter_queue_on_output_queue(\u0027stderr\u0027)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"    def _test__kill(self, respawning, pid\u003dNone):"},{"line_number":158,"context_line":"        with contextlib.nested("},{"line_number":159,"context_line":"                mock.patch.object(self.proc, \u0027_kill_event\u0027),"},{"line_number":160,"context_line":"                mock.patch.object(utils, \u0027get_child_pid\u0027,"},{"line_number":161,"context_line":"                                  return_value\u003dpid),"}],"source_content_type":"text/x-python","patch_set":10,"id":"baa201ad_28894675","line":158,"updated":"2014-10-15 15:38:28.000000000","message":"contextlib.nested is deprecated. Now the with statement supports this behaviour without the need of the error prone \u0027nested\u0027. As an alternative, there\u0027s ExitStack for Python 3 (which was ported on 2 as I remember).","commit_id":"75a8c83fccb7bc0c6d4e3ab2e782bc0498d79f8e"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"d3f3d760c70f4f2187376b196d993a9e0a993c35","unresolved":false,"context_lines":[{"line_number":155,"context_line":"        self._test_iter_output_calls_iter_queue_on_output_queue(\u0027stderr\u0027)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"    def _test__kill(self, respawning, pid\u003dNone):"},{"line_number":158,"context_line":"        with contextlib.nested("},{"line_number":159,"context_line":"                mock.patch.object(self.proc, \u0027_kill_event\u0027),"},{"line_number":160,"context_line":"                mock.patch.object(utils, \u0027get_child_pid\u0027,"},{"line_number":161,"context_line":"                                  return_value\u003dpid),"}],"source_content_type":"text/x-python","patch_set":10,"id":"baa201ad_9d1ed96e","line":158,"in_reply_to":"baa201ad_28894675","updated":"2014-10-16 11:26:02.000000000","message":"Wow, I didn\u0027t know that. Thanks!","commit_id":"75a8c83fccb7bc0c6d4e3ab2e782bc0498d79f8e"},{"author":{"_account_id":8873,"name":"Assaf Muller","email":"amuller@redhat.com","username":"amuller"},"change_message_id":"a5c1c972fe35d6e01b0b406a92ca186f3223e697","unresolved":false,"context_lines":[{"line_number":155,"context_line":"        self._test_iter_output_calls_iter_queue_on_output_queue(\u0027stderr\u0027)"},{"line_number":156,"context_line":""},{"line_number":157,"context_line":"    def _test__kill(self, respawning, pid\u003dNone):"},{"line_number":158,"context_line":"        with contextlib.nested("},{"line_number":159,"context_line":"                mock.patch.object(self.proc, \u0027_kill_event\u0027),"},{"line_number":160,"context_line":"                mock.patch.object(utils, \u0027get_child_pid\u0027,"},{"line_number":161,"context_line":"                                  return_value\u003dpid),"}],"source_content_type":"text/x-python","patch_set":10,"id":"baa201ad_c31de263","line":158,"in_reply_to":"baa201ad_9d1ed96e","updated":"2014-10-16 12:05:36.000000000","message":"We\u0027re using contextlib.nested throughout the code because of Python 2.6, it doesn\u0027t support an aggregated with statement. I have a vague recollection of dropping 2.6 support for K but this would have to be triple checked.","commit_id":"75a8c83fccb7bc0c6d4e3ab2e782bc0498d79f8e"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":172,"context_line":"            else:"},{"line_number":173,"context_line":"                self.assertIsNone(self.proc._kill_event)"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            mock_get_child_pid.assert_called_once_with("},{"line_number":176,"context_line":"                self.proc._process.pid,"},{"line_number":177,"context_line":"                self.proc.root_helper)"},{"line_number":178,"context_line":"        mock_kill_event.send.assert_called_once_with()"}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_bd37fdfd","line":175,"updated":"2014-10-27 17:17:20.000000000","message":"Revisiting this code, it\u0027s pretty clear that we\u0027re asserting calls in a way that isn\u0027t very useful.  I think my intent was to ensure coverage of the branches involved, but functional coverage is far preferable than this duplication of the method\u0027s logic.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":172,"context_line":"            else:"},{"line_number":173,"context_line":"                self.assertIsNone(self.proc._kill_event)"},{"line_number":174,"context_line":""},{"line_number":175,"context_line":"            mock_get_child_pid.assert_called_once_with("},{"line_number":176,"context_line":"                self.proc._process.pid,"},{"line_number":177,"context_line":"                self.proc.root_helper)"},{"line_number":178,"context_line":"        mock_kill_event.send.assert_called_once_with()"}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_0e66b551","line":175,"in_reply_to":"9aa7fdbe_bd37fdfd","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":188,"context_line":"    def test__kill_targets_process_for_pid(self):"},{"line_number":189,"context_line":"        self._test__kill(False, pid\u003d\u00271\u0027)"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def test__kill_with_root_helper(self):"},{"line_number":192,"context_line":"        self.proc.root_helper \u003d \u0027foo\u0027"},{"line_number":193,"context_line":"        self._test__kill(False)"},{"line_number":194,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_f870b3a5","line":191,"updated":"2014-10-27 17:17:20.000000000","message":"I don\u0027t know what value this adds - the functional tests will validate this case.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":188,"context_line":"    def test__kill_targets_process_for_pid(self):"},{"line_number":189,"context_line":"        self._test__kill(False, pid\u003d\u00271\u0027)"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"    def test__kill_with_root_helper(self):"},{"line_number":192,"context_line":"        self.proc.root_helper \u003d \u0027foo\u0027"},{"line_number":193,"context_line":"        self._test__kill(False)"},{"line_number":194,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_2e5b7188","line":191,"in_reply_to":"9aa7fdbe_f870b3a5","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"}],"neutron/tests/unit/test_agent_linux_utils.py":[{"author":{"_account_id":7787,"name":"Kevin Benton","email":"kevin@benton.pub","username":"blak111"},"change_message_id":"ebbc92030db6c3008ba2eb6cc86c464eb040e050","unresolved":false,"context_lines":[{"line_number":174,"context_line":""},{"line_number":175,"context_line":""},{"line_number":176,"context_line":"class TestGetPidToKill(base.BaseTestCase):"},{"line_number":177,"context_line":"    def _test_get_child_pid(self, expected\u003d_marker,"},{"line_number":178,"context_line":"                            root_helper\u003dNone, pids\u003dNone):"},{"line_number":179,"context_line":"        def _find_child_pids(x):"},{"line_number":180,"context_line":"            if not pids:"}],"source_content_type":"text/x-python","patch_set":2,"id":"da9df570_0e6c1a9c","line":177,"updated":"2014-09-21 00:18:49.000000000","message":"I don\u0027t understand why you need a global tuple to reference here. Why not expected\u003dNone and then check \u0027if expected is None\u0027 on line 189?","commit_id":"c8d20a37ec4e7bbfef05c45d62233a1cb90709f0"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"25cda7a095e0abb7af2a528f953e25ed73d96762","unresolved":false,"context_lines":[{"line_number":174,"context_line":""},{"line_number":175,"context_line":""},{"line_number":176,"context_line":"class TestGetPidToKill(base.BaseTestCase):"},{"line_number":177,"context_line":"    def _test_get_child_pid(self, expected\u003d_marker,"},{"line_number":178,"context_line":"                            root_helper\u003dNone, pids\u003dNone):"},{"line_number":179,"context_line":"        def _find_child_pids(x):"},{"line_number":180,"context_line":"            if not pids:"}],"source_content_type":"text/x-python","patch_set":2,"id":"da9df570_d9d015fa","line":177,"in_reply_to":"da9df570_0e6c1a9c","updated":"2014-09-22 15:29:46.000000000","message":"This test is taken from tests for async_process. I think refactoring the code is out of scope of this patch.","commit_id":"c8d20a37ec4e7bbfef05c45d62233a1cb90709f0"},{"author":{"_account_id":8788,"name":"Miguel Angel Ajo","email":"mangelajo@redhat.com","username":"mangelajo"},"change_message_id":"cfef5bb98f2ed93fbf745a638bc0229990c66a21","unresolved":false,"context_lines":[{"line_number":186,"context_line":"        with mock.patch.object(utils, \u0027find_child_pids\u0027,"},{"line_number":187,"context_line":"                               side_effect\u003d_find_child_pids):"},{"line_number":188,"context_line":"            actual \u003d utils.get_child_pid(mock_pid, root_helper)"},{"line_number":189,"context_line":"        if expected is _marker:"},{"line_number":190,"context_line":"            expected \u003d mock_pid"},{"line_number":191,"context_line":"        self.assertEqual(expected, actual)"},{"line_number":192,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_4c0f5388","line":189,"updated":"2014-10-27 10:23:39.000000000","message":"I don\u0027t get the expected/_marker thing, may be I\u0027m missing a bit of coffee this morning.\n\nWouldn\u0027t be simpler to have expected\u003dNone as a parameter\nand later on\n\nexpected \u003d expected or mock_pid\n\n?","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":186,"context_line":"        with mock.patch.object(utils, \u0027find_child_pids\u0027,"},{"line_number":187,"context_line":"                               side_effect\u003d_find_child_pids):"},{"line_number":188,"context_line":"            actual \u003d utils.get_child_pid(mock_pid, root_helper)"},{"line_number":189,"context_line":"        if expected is _marker:"},{"line_number":190,"context_line":"            expected \u003d mock_pid"},{"line_number":191,"context_line":"        self.assertEqual(expected, actual)"},{"line_number":192,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_78fda3b0","line":189,"in_reply_to":"9aa7fdbe_2365bdb4","updated":"2014-10-27 17:17:20.000000000","message":"Given that you\u0027re rewriting it anyway, fixing deficiencies in the original code should be considered fair game.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"0a58d8822d0eecbd0e5589d170a719da541f11c5","unresolved":false,"context_lines":[{"line_number":186,"context_line":"        with mock.patch.object(utils, \u0027find_child_pids\u0027,"},{"line_number":187,"context_line":"                               side_effect\u003d_find_child_pids):"},{"line_number":188,"context_line":"            actual \u003d utils.get_child_pid(mock_pid, root_helper)"},{"line_number":189,"context_line":"        if expected is _marker:"},{"line_number":190,"context_line":"            expected \u003d mock_pid"},{"line_number":191,"context_line":"        self.assertEqual(expected, actual)"},{"line_number":192,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_2365bdb4","line":189,"in_reply_to":"9aa7fdbe_4c0f5388","updated":"2014-10-27 11:27:06.000000000","message":"I think the original reason here is to test None as relevant value returned by get_child_pid() (L205). If I would be the author, I would chose object() as _marker.","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":186,"context_line":"        with mock.patch.object(utils, \u0027find_child_pids\u0027,"},{"line_number":187,"context_line":"                               side_effect\u003d_find_child_pids):"},{"line_number":188,"context_line":"            actual \u003d utils.get_child_pid(mock_pid, root_helper)"},{"line_number":189,"context_line":"        if expected is _marker:"},{"line_number":190,"context_line":"            expected \u003d mock_pid"},{"line_number":191,"context_line":"        self.assertEqual(expected, actual)"},{"line_number":192,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_ae4e81bf","line":189,"in_reply_to":"9aa7fdbe_78fda3b0","updated":"2014-11-11 17:05:34.000000000","message":"Done","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":2035,"name":"Maru Newby","email":"marun@redhat.com","username":"maru"},"change_message_id":"9ec3f10c7ef54433a3e96d32e1cb098f9717cd50","unresolved":false,"context_lines":[{"line_number":197,"context_line":"        self._test_get_child_pid(expected\u003d\u00272\u0027, pids\u003d[\u00271\u0027, \u00272\u0027],"},{"line_number":198,"context_line":"                                 root_helper\u003d\u0027a\u0027)"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"    def test_get_child_pid_returns_last_child_pid_with_root_Helper(self):"},{"line_number":201,"context_line":"        self._test_get_child_pid(expected\u003d\u00273\u0027, pids\u003d[\u00271\u0027, \u00272\u0027, \u00273\u0027],"},{"line_number":202,"context_line":"                                 root_helper\u003d\u0027a\u0027)"},{"line_number":203,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"9aa7fdbe_d825af5c","line":200,"updated":"2014-10-27 17:17:20.000000000","message":"Helper -\u003e helper","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"},{"author":{"_account_id":8655,"name":"Jakub Libosvar","email":"libosvar@redhat.com","username":"jlibosva"},"change_message_id":"6ad31fd6a1d38b1c344dc197d9a6ec83898ff5d9","unresolved":false,"context_lines":[{"line_number":197,"context_line":"        self._test_get_child_pid(expected\u003d\u00272\u0027, pids\u003d[\u00271\u0027, \u00272\u0027],"},{"line_number":198,"context_line":"                                 root_helper\u003d\u0027a\u0027)"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"    def test_get_child_pid_returns_last_child_pid_with_root_Helper(self):"},{"line_number":201,"context_line":"        self._test_get_child_pid(expected\u003d\u00273\u0027, pids\u003d[\u00271\u0027, \u00272\u0027, \u00273\u0027],"},{"line_number":202,"context_line":"                                 root_helper\u003d\u0027a\u0027)"},{"line_number":203,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5a890539_6e265918","line":200,"in_reply_to":"9aa7fdbe_d825af5c","updated":"2014-11-11 17:05:34.000000000","message":"Good eye, I thought pep8 check would catch that...it\u0027s copied from neutron/tests/unit/agent/linux/test_async_process.py","commit_id":"d7d66998053d1160153fac8049280c19a39b7538"}]}