John Fischer wrote: > Joe, > > Uhm... yeah. That's embarrassing. That's not so embarrassing! Stick around and watch the mistakes I make! ;)
> > Updated webrev at: > > http://cr.opensolaris.org/~johnfisc/path-7325/ Looks OK to push now. Thanks for working through the issues of this with me! Joe > > Thanks, > > John > > Joseph J VLcek wrote: >> These still seem to have the ${PATH} >> >> >> e.g.: >> >> 34 PATH=/usr/bin:/usr/sbin:/sbin:/usr/lib/installadm:${PATH}; export >> PATH >> >> >> Joe >> >> >> >> >> John Fischer wrote: >>> 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 >>>> >> >> >>
