Hi Karen.
On 03/16/12 16:13, Karen Tung wrote:
Hi Jack,
Please see my comments inline. I removed all items where I have no
further comments.
On 03/16/12 03:06 PM, Jack Schwartz wrote:
usr/src/cmd/system-config/profile/support_info.py:
- The _fork_proc() function:
My understanding of this function is that you want to be able
to kill the process after the specified timeout. If that's the case,
why is it necessary to have all the logic to do non-blocking read
of the stdout and stderr? You are not processing any of the
information
you read from stdout and stderr anyway
While it is true that I am not processing info while I am looping,
the stdout "pipe" can fill up depending on the output of ocmadm and
asradm. (Some of the properties are large, and together could
overrun the pipe depending on the pipe size.) By reading, I
continuously empty the pipe so that it won't fill up. Deadlock will
occur if the pipe fills up and more the subprocess tries to write more.
Did you take a look at the bufsize argument for Popen? I think your
concern can be addressed
by setting bufsize=-1, so, all the output is fully buffered.
I experimented with different bufsizes but it did not work. Whether I
set the bufsize to 0 or -1, or a large enough buffer, the subprocess
would not terminate until it was all read. I tried using both
subprocess.poll() and os.waitpid() to check for process termination.
If you know of another way to check for termination which will allow me
to skip reading between polls, I'll do it. However, as I see it, bottom
line: in order to see when the process terminates before the timeout, I
need to read the pipe in between polls.
- lines 367-368: can this be:
proxy_hostname += ":" + self.proxy_port
I thought using + for strings was discouraged for performance
reasons, and join was to be used instead.
I do not know about the performance difference between using "+" or
using "join". I don't remember seeing
too much code that uses join for string concatenation. That's why I
thought it looks kinda odd.
Actually, join is the right thing to use, but I was using it
incorrectly. What I had will work, but:
":".join([proxy_hostname, self.proxy_port])
is more appropriate. It uses the ":" to join the two arguments. I made
this change.
Webrev updated with delta from V3:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7104766_4b_3/webrev.review4b/
Thanks,
Jack
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
