----- lookkas@xxxxxxxxx wrote: > 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. Note that pexpect has features that kvm_subprocess doesn't -- if I remember correctly it can log the output to a user specified text file, and uses an internal buffer that may allow for some more flexibility in reading from the STDOUT/STDERR pipe, and maybe other things. The KVM test doesn't need these things, but if you're seriously considering using kvm_subprocess outside the KVM test, make sure you don't need these things either. > 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. I agree. (It seemed harmless to catch all exceptions there because setting those descriptors to None eventually leads to failure, but still I agree it's safer to catch only the expected ones.) > 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? I don't think so. In what cases do we want to distinguish stdout and stderr? Does pexpect make the distinction? > 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? I wanted to keep close() simple (unlike VM.destroy()). I can add a timeout however and a parameter to control whether SIGKILL should be sent automatically to misbehaving processes. BTW, I wonder if we should add something like a 'close_command' parameter, which will specify a command to send before closing the process. In the case of SSH/Telnet this should be "exit", and in the case of QEMU it should be disabled. If we do that, however, I'd rather do it in a future patch, because I want it to undergo some testing. > 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? No, because the os.kill(pid, 0) test makes sure the PID exists. The second test is there just to make sure the PID doesn't belong to the wrong process. If for some reason we can't find the PID under /proc, I think we can safely say we passed the second test. It's OK to return False if we can't find the PID under /proc, but only if we can guarantee that all PIDs are listed under /proc as soon as they come into existence. In other words, we need to know for sure that the equality `os.kill(PID, 0) succeeds` == `PID exists under proc` is true at all times, atomically (particularly, that no side of the equality becomes true before the other). Since I wasn't sure about that, I preferred to return True, which is harmless anyway. > 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 > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
