Joe, Opps... It should be:
http://cr.opensolaris.org/~johnfisc/path-7325/ Thanks, John Joseph J VLcek wrote: > John Fischer wrote: >> Joe, >> >> Updated webrev at: >> >> http://cr.opensolaris.org/~johnf/path-7325 > > John, > > The webrev is not there. > > Joe > >> >> This time without the installadm_util.c change. >> >> Thanks, >> >> John >> >> John Fischer wrote: >>> Joe, >>> >>> I'll drop it then. >>> >>> John >>> >>> Joseph J. VLcek wrote: >>>> Thanks John! >>>> >>>> >>>> My comments below. >>>> >>>> Joe >>>> >>>> John Fischer wrote: >>>>> Joe, >>>>> >>>>> Thanks!! >>>>> >>>>> John >>>>> >>>>> Joseph J. VLcek wrote: >>>>>> Hey John, >>>>>> >>>>>> usr/src/cmd/installadm/installadm_util.c >>>>>> Still have the PATH setting logic. Can't this go away now? >>>>> >>>>> Yes. But... I wanted to be certain that should the function >>>>> (installadm_system()) be used in another way (i.e., not to fork >>>>> one of the scripts) that we got the correct PATH in that case >>>>> as well. Am I being overly cautious? >>>> >>>> Hum... I see your logic but I think it is not necessary. >>>> >>>>> >>>>>> >>>>>> All the scripts have: >>>>>> PATH=/usr/bin:/usr/sbin:/sbin:/usr/lib/installadm:${PATH}; export >>>>>> PATH >>>>>> >>>>>> I don't think you what the ${PATH}. >>>>>> >>>>>> I think it should be: >>>>>> >>>>>> # >>>>>> # Establish PATH for non-built in commands >>>>>> # >>>>>> export PATH=/usr/bin:/usr/sbin:/sbin:/usr/lib/installadm >>>>>> >>>>> >>>>> I will drop the $PATH. >>>>> >>>>>> Joe >>>>>> >>>>>> >>>>>> >>>>>> John Fischer wrote: >>>>>>> Joe, >>>>>>> >>>>>>> Thanks for your help and patience. >>>>>>> >>>>>>> I have finally updated the webrev for this bug. I now have the >>>>>>> scripts setting the PATH appropriately. The scripts now contain >>>>>>> the line: >>>>>>> >>>>>>> PATH=/usr/bin:/usr/sbin:/sbin:/usr/lib/installadm:${PATH}; export >>>>>>> PATH >>>>>>> >>>>>>> The new webrev is located at: >>>>>>> >>>>>>> http://cr.opensolaris.org/~johnfisc/path-7325/ >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> John >>>>>>> >>>>>>> Joseph J. VLcek wrote: >>>>>>>> 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 >>>>>>>> >>>>>>>> >>>>>> >>>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> caiman-discuss at opensolaris.org >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
