Hi Evan.
Thanks for reviewing.
On 07/14/10 03:39 PM, Evan Layton wrote:
Hi Jack,
I do still have some concerns with the complexity of be_run_cmd()
and how maintainable it will be. However as we discussed I don't
have a better suggestion on dealing with the need to capture stdout
and stderr separately.
The other alternative, which we discussed at our staff meeting a few
weeks ago, is to write stderr to a file, and read the file. But that's
hack-amundo...
Note that a significant part of the code is for error handling, ~74
lines out of 180, but that's just the way C is.
Poll needs to be used when reading both (stdout and stderr) pipes at
once. The alternative would be to read the two pipes serially, say
stderr first and then stdout. If the subprocess was running and stderr
was being read, but the stdout pipe filled up before the subprocess
finished, the subprocess would hang if it needed to write to stdout.
This would create a deadlock, where the parent would be waiting for the
child to finish, but the child would be waiting for the parent to read
stdout.
One nit related to my maintainability comment above is that it
might be helpful to put a comment block (starting at line 3011)
that gives a description of how the pollfds stuff works for
grabbing both stdout and strerr.
OK. Will do.
As per our phone conversation I'm fine with this updated set of
changes.
Also per our conversation, I will put the snprintfs of lines 3016 and
3040 inside the IFs below them. This is not really needed*, but it
would help make the code make more sense.
* In the case of stdxxx_buf being NULL, stdxxx_buf_remaining will be
zero, so snprintf won't do anything when the IF condition would be false.
Webrev respun: same place:
http://cr.opensolaris.org/~schwartz/100710.1/webrev/
Delta webrev with these changes (only be_utils.c is interesting):
http://cr.opensolaris.org/~schwartz/100710.1/webrev.2.3.diff/
Thanks again,
Jack
Thanks,
-evan
On 7/10/10 5:45 PM, Jack Schwartz wrote:
Hi everyone.
This webrev supercedes the one I published a week ago, because the
situation with SNAP->ON integration has changed. (Thanks to those who
reviewed the last one!) It also includes some new changes.
Webrev:
http://cr.opensolaris.org/~schwartz/100710.1/webrev/
Webrev vs old one (for those who reviewed the last one):
http://cr.opensolaris.org/~schwartz/100710.1/webrev.diff/
Please review by Weds 7/14 COB.
Changes:
- Since we cannot be sure 6944352, which addresses changes we needed for
installboot and installgrub, would be done in time to make the ON cutoff
for feature freeze, bugs related to that fix have been removed. Added is
support for labeled branded zones, and fixes to be_create_menu to return
proper errors.
- Error message for BE_ERR_UNKNOWN is not "Unknown error" not "Unknown
external error" as all external commands are now covered by other
errors.
- Cleanup of be_create_menu() error handling for 16512 is new.
- Addition in libbe.h of "labeled" in BE_ZONE_SUPPORTED_BRANDS for 13865
is new.
Note: The new be_run_cmd function in be_utils.c (the bulk of original
be_utils.c changes) has not changed.
Tested:
Labeled zones:
- installed trusted solaris and created a zone. Then verified that the
zone (and files it had) was properly handled in a new BE.
- Ran regression test of libbe test suite from the STC gate and verified
that there were no new errors.
Messaging of external commands:
Hid ict.py, deleted menu.lst and tried to create a new menu to check
error output if splash screen could not be added to new menu.lst.
Hid installgrub, changed capability file to force installgrub call, and
then did beadm activate to check error output when installgrub could not
be executed.
Modules I changed are lint clean. All files are hg nits clean.
Thanks for your time,
Jack
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
