Hi Joe.
Thanks for your review.
On 07/12/10 02:13 PM, [email protected] wrote:
On 07/10/10 07: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
Hey Jack,
Here's my input. Hope this helps. I realize we discussed some of
this already but I listed my comments so others could see them and
perhaps add to them.
Joe
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/libbe/be_activate.c
Suggestion to consider:
-----------------------
Would it maybe be better to update be_print_err() to invoke gettext?
e.g.:
Change from
833 be_print_err(gettext(...
To:
833 be_print_err(...
And update be_print_err() to invoke gettext.
There are now cases where we don't need to call gettext() to
be_print_err() input, for example when we pass be_print_err() error
messages from external commands.
It's also worthy to note that current error handling will eventually
be replaced by the error library, so it's not worth doing from that
standpoint as well.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
usr/src/lib/libbe/be_utils.c
Idea:
----
2936 /* Handle if caller doesn't want stdout or stderr output. */
2937 if (stdout_buf == NULL) {
2938 stdout_buf_remain = 0;
2939 }
2940 if (stderr_buf == NULL) {
2941 stderr_buf_remain = 0;
2942 }
1st off I like this code the way it is because it continues if
it can. However if the user specifies a buffer size but a NULL
buffer maybe it would be less confusing if the code didn't
continue and returned return (BE_ERR_EXTCMD);
if ((stdout_buf == NULL) && (stdout_bufsize != 0)) {
return (BE_ERR_EXTCMD);
}
if ((stderr_buf == NULL) && (stderr_bufsize != 0)) {
return (BE_ERR_EXTCMD);
}
Just something to think about. Either way is OK with me.
It is a valid use case for stdout and/or stderr to be
unspecified/unused. This is in the header comments for be_run_cmd.
Issue:
------
2871 * Function: be_run_cmd
This solution, although feature rich, seems heavy handed to me.
While reviewing this I wrote a simple C program using popen() so
I understand the problem with capturing stderr using it from C.
But Python does a good job of doing this already perhaps
implementing this in Pyhon might result in simpler code.
So the problem I am trying to solve here is to run a subprocess and
be able to read both its stdout and stderr if needed, in a non-hacky
lightweight implementation. A few weeks ago at the staff meeting I
asked the group about my options here. I presented two:
1) read both stdout and stderr from a subprocess by a simple but
hacky solution of routing stderr to a file, and then reading the file.
2) a more complex implementation of a multi-pipe popen using
low-level system service calls (like dup(2), fork(2) and exec(2).
Neither solution was favored. Out of that discussion came a
mid-level-complexity solution of popen(3C) and mkfifo(3C); this was
implemented as it was less complex and not hacky.
Your suggestion of doing error handling for a C program in python is
another option, but it seems quite heavyweight to me. We would have
to use a C-python bridge and start up a python process just to
process errors. We would have to handle the errors that the python
error handler itself could return.
be_run_cmd() is larger than the other similar functions in
slim_source because it has to handle two pipes, not just one. With
one pipe, one process can simply spin waiting to read the single
pipe while the subprocess submits text to the pipe as it does its
thing. With two pipes, however, poll(2) needs to be used to
coordinate their being read.
Or using popen() and directing stderr to a file that could be read
after the command completed.
Frankly I'm surprised that libc doesn't have a multi-pipe popen-ish
implementation like python does. If it did, I wouldn't have to write
one myself...
I thought through my solution and like it for the reasons above.
That said, if something better comes along, I'm open to it.
Issue:
------
Typo returing -> returning
2909 * - There were errors extracting or returing subprocess
Thanks, fixed.
Thanks,
Jack
