Joe,
Updated webrev at:
http://cr.opensolaris.org/~johnf/path-7325
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
