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?
>
> 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
>>>
>>>
>
