From mboxrd@z Thu Jan 1 00:00:00 1970 From: lookkas@gmail.com Subject: Adding kvm_subprocess Date: Thu, 18 Jun 2009 08:55:41 +0000 Message-ID: <001636283b0a8e4fd2046c9b9224@google.com> Reply-To: lookkas@gmail.com, michael goldish , autotest@test.kernel.org, kvm@vger.kernel.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed; delsp=yes Content-Transfer-Encoding: 7bit Cc: michael goldish , autotest@test.kernel.org, kvm@vger.kernel.org To: lookkas@gmail.com Return-path: Received: from mail-gx0-f229.google.com ([209.85.217.229]:44626 "EHLO mail-gx0-f229.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753560AbZFRJXN (ORCPT ); Thu, 18 Jun 2009 05:23:13 -0400 Received: by gxk13 with SMTP id 13so1532368gxk.1 for ; Thu, 18 Jun 2009 02:23:15 -0700 (PDT) Sender: kvm-owner@vger.kernel.org List-ID: Reviewers: lmr, Message: Hi Michael, this is my first review of your patch series. The module kvm_subprocess looks pretty good and carefully written, I made some minor comments (some of them are more of general wonderings). After reviewing this, I began to think seriously about replacing pexpect by this library. Thank you very much for your work on this, http://codereview.appspot.com/79042/diff/1/4 File client/tests/kvm/kvm_subprocess.py (right): http://codereview.appspot.com/79042/diff/1/4#newcode142 Line 142: except: We probably want to catch AssertionError and OsError here. http://codereview.appspot.com/79042/diff/1/4#newcode215 Line 215: """ Would it be possible to distinguish stdout and stderr when reading from the process? http://codereview.appspot.com/79042/diff/1/4#newcode238 Line 238: pass The function sends by default SIGTERM to the process. In such cases, what do we do with misbehaving (hang) processes? Just leave it as it is and close other file descriptors? Wouldn't be interesting to send a SIGKILL if another signal doesn't work? http://codereview.appspot.com/79042/diff/1/4#newcode279 Line 279: return True Isn't risky to just return True if we can't find the PID under /proc? Description: The following patches replace the run_bg() function and the kvm_spawn class with kvm_subprocess. The new module is intended to solve a problem with run_bg() (which loses track of the child process when the parent process exits) and allows for more flexibility in handling SSH/Telnet sessions (allows reusing sessions started in previous tests). kvm_subprocess defines a class 'kvm_spawn' and two functions: run_bg() and run_fg(). Its main job is to run a process in the background and allow the user to control it interactively and/or monitor its output on the fly. Its most important feature in the context of KVM tests is that it allows to control child processes left behind by processes that no longer exist. This means that if QEMU is started by the 'boot' test, its output will be logged in the following 'reboot' test as well. See kvm_spawn's docstring for more details. Please review this at http://codereview.appspot.com/79042 Affected files: M client/tests/kvm/kvm_preprocessing.py A client/tests/kvm/kvm_subprocess.py M client/tests/kvm/kvm_tests.py M client/tests/kvm/kvm_utils.py M client/tests/kvm/kvm_vm.py