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