On 02/19/09 02:57, [email protected] wrote:
The webrev http://cr.opensolaris.org/~padraig/ips-6549-v1/ fixed
6549 Defunct subprocess of packagemanager
This is a simple fix which just adds a call to proc.wait() on the
subprocess.
A few comments here.
- Do you want / need to check the process return code that is returned
by wait()? There's some amount of error handling here that you might
be able to perform by checking the return code instead of parsing the
output from stderr.
We do not want or need to check the process return code.
We are examining the usage statement returned by beadm to determine
whether -F is supported for beadm destroy.
- If you do need to parse stderr, there are some alternatives to the
current approach. Popen.communicate() returns a (stdout, stderr)
tuple and waits for the process to terminate. If the output from
beadm is expected to be small, this would be the way to go.
I have regenerated the webrev to use communicate. See
http://cr.opensolaris.org/~padraig/ips-6549-v2/.
Padraig
- If you want to read stderr directly, as the code does now, the
stderr.read() should go before the proc.wait(). Since the beadm
command is writing its stderr to a pipe, this write can block once the
pipe gets filled. If you don't drain the pipe until wait() returns,
it's possible that the process won't ever exit, since it's blocked
while writing the error message.
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
