----- "Lucas Meneghel Rodrigues" <lmr@xxxxxxxxxx> wrote: > 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. kvm_subprocess is a little weird in that it does two different things -- handling of both non-interactive subprocesses and SSH sessions. With this approach I don't think we need to write an SSH subclass of kvm_spawn because it already does a lot of SSH stuff. If we do any subclassing at all -- I suggest that we remove all the SSH stuff from kvm_spawn and put it in a subclass somehow, so that kvm_spawn natively only handles non-interactive subprocess (with _tail(), get_output(), get_status() etc), and the subclass does everything else (read_up_to_prompt(), get_command_status_output() etc). ssh_login() can remain an external function that creates and returns a kvm_spawn object. This will have to be done carefully because each 'user' of the kvm_spawn server needs a named pipe of its own, which will have to be handled by the constructor. If you think this is a good idea I'd rather make the necessary changes to kvm_subprocess myself. Does this make sense to you, or did I misunderstand what you meant by writing an SSH subclass? > > > 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. OK. I try to test it as much as possible by using it exclusively instead of the previous run_bg() and kvm_spawn. > > > 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
