On Thu, 2009-06-18 at 07:58 -0400, Michael Goldish wrote: > ----- 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. That's precisely the point, pexpect and pxssh are external gpl modules that we bundled to be able to use a python library to do interaction with interactive programs. For most of the tests we don't need the fancy stuff of pexpect, so it'd be nice to have a solution grown inside autotest. I will see if I can write a ssh subclass of kvm_spawn, make the necessary arrangements and verify if everything is working OK. > > 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.) Yes, I also agree that it is fairly harmless. Pointed out just for the sake of the review :) > > 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? It's usually nice to tell the difference between the two of them for debugging purposes, but nothing I'd consider critical. pexpect uses a similar approach (non blocking read using select), and I realize it doesn't. > > 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. That sounds good, and I don't think it will over complicate the function. > 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. No rush, let's keep the whole module under testing a little bit more. > > 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. Ok, fair enough! Other than the very minor stuff we've talked about, we are good to go. -- Lucas Meneghel Rodrigues Software Engineer (QE) Red Hat - Emerging Technologies -- 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
