public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
From: lookkas@gmail.com
To: lookkas@gmail.com
Cc: michael goldish <mgoldish@redhat.com>,
	autotest@test.kernel.org, kvm@vger.kernel.org
Subject: Adding kvm_subprocess
Date: Thu, 18 Jun 2009 08:55:41 +0000	[thread overview]
Message-ID: <001636283b0a8e4fd2046c9b9224@google.com> (raw)

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



             reply	other threads:[~2009-06-18  9:23 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-06-18  8:55 lookkas [this message]
     [not found] <1066125537.201511245326193976.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com>
2009-06-18 11:58 ` Adding kvm_subprocess Michael Goldish
     [not found] <782493788.202091245327169940.JavaMail.root@zmail05.collab.prod.int.phx2.redhat.com>
2009-06-18 12:19 ` Michael Goldish

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=001636283b0a8e4fd2046c9b9224@google.com \
    --to=lookkas@gmail.com \
    --cc=autotest@test.kernel.org \
    --cc=kvm@vger.kernel.org \
    --cc=mgoldish@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox