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