Hey John,
usr/src/cmd/installadm/installadm_util.c
Still have the PATH setting logic. Can't this go away now?
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
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
>>
>>
