David.Comay at Sun.COM wrote:
[snip]
> usr/src/lib/libc/port/regex/wordexp.c
General note for this file: The new implementation for |workexp()| is a
"clone" of the original Solaris |wordexp()| implementation (and
therefore shares lots of coding+style issues) - I copied the whole
function and only did some minor modifications (e.g. old Solaris ksh
uses a secret "switch" to do the word expansion while the new code
passes a small mini-script to ksh93). I would really prefer to do only
minor changes to this function since testing the changes is
compliciated+risky and I'd like to keep the code in a state where we can
compare it against the original version - if this function fails or
crashes for whatever reasons the SMF service may fail (or crash),
leaving the system in some kind of unrecoverable zombie state (e.g. no
shutdown, reboot, remote login etc.; lucky users _may_ get console
access to fix the issue (which is non-trivial since the code sits in
libc)) which is difficult to "fix" for normal users (that's one reason
why we have this code in the codebase - the various OpenSolaris
distributions had trouble getting this right).
> Line 22 - Update copyright year.
Fixed.
> Lines 89-90 - Cstyle error - Please put the while on the same
> line as the closing curly brace
>
> } while (*s++ != '\0');
Fixed (by re-implementing |stpcpy()| based on Solaris's
|libc::strcpy()|). The new code looks like this:
-- snip --
/*
* |mystpcpy| - like |strcpy()| but returns the end of the buffer
* We'll add this later (and a matching multibyte/widechar version)
* as normal libc function.
*
* Copy string s2 to s1. s1 must be large enough.
* return s1-1 (position of string terminator ('\0') in destnation
buffer).
*/
char *
mystpcpy(char *s1, const char *s2)
{
while (*s1++ = *s2++)
;
return (s1-1);
}
-- snip --
> Line 117 - s/neccessary/necessary/
Fixed.
> Line 135 - There seem to be multiple spaces between the words
> in several parts of this line.
AFAIK that comes from the original implementation (e.g. that comment was
cloned).
Fixed.
> Line 144 - s/allocting/Allocate/
>
> Line 151 - Cstyle error - continuation lines should be indented
> four spaces.
Uhm... cstyle doesn't complain:
-- snip --
$ cstyle -P
/home/gisburn/ksh93/on_build1/test1_x86/usr/src/lib/libc/port/regex/wordexp.c
$ cstyle -p
/home/gisburn/ksh93/on_build1/test1_x86/usr/src/lib/libc/port/regex/wordexp.c
$ cstyle -pP
/home/gisburn/ksh93/on_build1/test1_x86/usr/src/lib/libc/port/regex/wordexp.c
$ cstyle
/home/gisburn/ksh93/on_build1/test1_x86/usr/src/lib/libc/port/regex/wordexp.c
-- snip --
> Lines 217-226 - Could you provide more details here? Is
> /usr/lib/libc/libc_wordexp_commands another interface that can
> be customized by users?
No, it is not intended as official interface. The path could be named
/chicken/monster/has/rabies/ or something like that (pointing to a
non-existing location to prevent the execution of path-bound commands
when the shell runs in restricted mode (the only commands which can be
used are "print" and "sleep" which are safe to use, e.g. both cannot be
used to escape the restricted shell "jail")). ${PATH} is set to a
location which is more or less "guranteed" to be not accessible by
normal users and IMO the ${PATH} element should have a descriptive name.
> Lines 308, 310, 312 - Is there any header file which #defines
> these ksh93 exit codes?
Unfortunately no (I added that to my ToDo list (not for this putback)).
----
Bye,
Roland
--
__ . . __
(o.\ \/ /.o) roland.mainz at nrubsig.org
\__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer
/O /==\ O\ TEL +49 641 7950090
(;O/ \/ \O;)
