Joe,
Uhm... yeah. That's embarrassing.
Updated webrev at:
http://cr.opensolaris.org/~johnfisc/path-7325/
Thanks,
John
Joseph J VLcek wrote:
> These still seem to have the ${PATH}
>
>
> e.g.:
>
> 34 PATH=/usr/bin:/usr/sbin:/sbin:/usr/lib/installadm:${PATH}; export PATH
>
>
> Joe
>
>
>
>
> John Fischer wrote:
>> Joe,
>>
>> Opps... It should be:
>>
>> http://cr.opensolaris.org/~johnfisc/path-7325/
>>
>> Thanks,
>>
>> John
>>
>> Joseph J VLcek wrote:
>>> 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
>>>
>
>
>
