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
