John Fischer wrote: > Joe, > > Thanks for taking the time to review the webrev. > > It is fairly simple to add the path this way. I think that the > best thing to do is have a standard of how the underlying scripts > are written. I am bent towards always full qualifying every > command from my security days in the desktop. > > If the scripts will be used directly from the command line or if > another fork/exec mechanism is developed within the installadm command > then you are absolutely correct in saying that there will be additional > problems. > > Looking at the scripts I see the following: > > 1) VARIABLE=<some binary> > ... > $VARIABLE ... > > i.e., GREP=/usr/bin/grep > $GREP "something" /etc/hosts > > 2) <some binary> # No PATH set > > i.e., grep "something" /etc/hosts > > 3) <some binary> # PATH set > > i.e., PATH=/usr/bin:/usr/sbin:/sbin:$PATH > grep "something" /etc/hosts > > 4) <fully path-ed binary> > > i.e., /usr/bin/grep "something" /etc/hosts > > The problem is that I see all 4 of these being used within the > scripts (actually sometimes all 4 in the same script) which is > probably from different developer's styles. > > Adding the PATH to all the scripts is also a simple solution and > could become the standard way we write scripts within the > component. > > Are you recommending that I modify the solution to add PATH to > each of the scripts? > > Thanks, > > John > > Joseph J. VLcek wrote: >> John Fischer wrote: >>> All, >>> >>> Fairly straight forward fix to a PATH issue. Low hanging fruit for the >>> new guy. More exciting things to come next week. ;-) >>> >>> webrev: >>> >>> http://cr.opensolaris.org/~johnfisc/path-7325/ >>> >>> Open Solaris defect: >>> >>> 7325 - installadm script should set PATH >>> >>> The fix is to use putenv() of a sensible path which is /usr/bin, >>> /usr/sbin and /usr/lib/installadm. Only set once for the process >>> via an initialize variable. >>> >>> When someone has a chance I would appreciate a review. >>> >>> Thanks, >>> >>> John >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> John, >> >> I think having the installadm_system() function in >> usr/src/cmd/installadm/installadm_util.c export a PATH is an simple >> and elegant solution. However I think it might be safer in the long >> run to have each script define PATH. >> >> This would make each script more robust and able to stand on it's own >> without the requirement it be invoked by installadm_system(). >> >> My concern is what happens if in the future a code change is made to >> have the scripts no longer invoked by installadm_system(). >> >> What do you think? >> >> Joe >> >> >> installadm_system >> Hey John,
Yes I would suggest adding PATH to each script. There is already a standard but as you point out it hasn't alway been followed. The standard can be seen here: http://hub.opensolaris.org/bin/view/Community+Group+on/shellstyle Which describes: There are two acceptable ways to do this: (1) make *all* command references in your script use explicit pathnames or (2) explicitly reset $PATH in your script. This is also documented here: http://installzone-wiki.central.sun.com/wiki/index.php/Ksh93_Tips and here: http://hub.opensolaris.org/bin/view/Project+shell/shellstyle#always_set_path As you point out, it is true that our scripts don't always follow this and sometimes mix methods. However I think when possible we should try to have our code adhere to this method. Which would lead us to have PATH defined in each script or alway use full path names. Some scripts indirectly use full path names with the CMD=<full path>; $CMD method. Joe
