RFR: 8067951: System.loadLibrary cannot find library when path contains quoted entry

[Ivan Gerasimov]

Thanks Sherman!

On 05.01.2015 22:10, Xueming Shen wrote:
>
> Just wonder if we really need that "inQuotes" logic here? A 
> straightforward approach might
> be "every time you have a quote, skip everything until you hit another 
> one, when counting,
> or copy everything into the buffer until hit another one, when copying" ?
>
I agree it would work, but, in my opinion, it would be a bit more 
complicated.
The counting loop would look something like this:
------------------------------------
         outerLoop: for (int i = 0; i < ldLen; ++i) {
             char ch = ldPath.charAt(i);
             if (mayBeQuoted && ch == '\"') {
                 thereAreQuotes = true;
                 for (++i; i < ldLen; ++i) {
                     if (ldPath.charAt(i) == '\"') {
                         continue outerLoop;
                     }
                 }
                 break; // unpaired quote
             } else if (ch == ps) {
                 psCount++;
             }
         }
------------------------------------
which is 3 lines longer, comparing to the loop with inQuotes flag.


> Since you are making copy (into the temporary buffer) anyway for 
> "quote" case, another
> approach might be to pre-scan the input propName first for quote, if 
> there is one, make
> a clean copy of the propName without those quotes, then go to the 
> original implementation.

Unfortunately, it wouldn't work in those rare cases, where the PATH 
contains a quoted path with semicolon in it.
Windows allows semicolon in the file names, and it seems to be the only 
reason to allow quoting the PATH entries.

Sincerely yours,
Ivan

> This logic can be implemented at the very beginning as
>
> private static String[] intializePath(String propname) {
>    if (ClassLoaderHelper.allowsQuotedPathElements) {
>        ...
>    }
>     ...   // existing implementation
> }
>
> -Sherman
>
> On 01/05/2015 10:10 AM, Roger wrote:
>> Hi Ivan,
>>
>> Looks pretty good.
>>
>> Just a thought on ClassLoader:1754;  If you avoid the local for 
>> ClassLoaderHelper.allowsQuotedPathElements
>> then the compiler can optimize the code and may do dead code 
>> elimination since it is a final static boolean.
>> On Mac and Unix it reduces to just the original code.
>>
>> (ClassLoaderHelper seems like a weak case for a separate class, IMHO)
>>
>> $.02, Roger
>>
>>
>> On 1/5/2015 12:49 PM, Ivan Gerasimov wrote:
>>> Hi Roger!
>>>
>>> I've updated the webrev similarly to what you've suggested:
>>> 1) Presence of quotes is checked when counting the separators.
>>> 2) If quotes weren't found, we'll execute the same optimized loop as 
>>> for Unix.
>>>
>>> http://cr.openjdk.java.net/~igerasim/8067951/4/webrev/
>>>
>>> Sincerely yours,
>>> Ivan
>>>
>>>
>>> On 05.01.2015 18:40, Roger wrote:
>>>> Hi Ivan,
>>>>
>>>> For this small difference in the implementations, I'd recommend 
>>>> against having
>>>> two different source files.  The path initialization function is a 
>>>> one time function and
>>>> the performance improvement is not significant.
>>>>
>>>>  I'd suggest a few comments on your 2nd version[1].
>>>>
>>>> - The windows check should check the system property or other 
>>>> definitive os check
>>>>   and could be better expressed (as Alan suggested) as quotesAllowed.
>>>> - in the loop testing for the quote (") can come before the 
>>>> quotesAllowed check to
>>>>   speed things up (no need to check if they are allowed if they do 
>>>> not occur).
>>>>   This code is unlikely to be executed enough times to optimized.
>>>>
>>>> Roger
>>>>
>>>> [1] http://cr.openjdk.java.net/~igerasim/8067951/2/webrev/
>>>>
>>>> On 1/4/2015 3:23 PM, Ivan Gerasimov wrote:
>>>>>
>>>>> On 04.01.2015 22:50, Alan Bateman wrote:
>>>>>> On 03/01/2015 17:39, Ivan Gerasimov wrote:
>>>>>>>
>>>>>>> Currently, there are tree variants of ClassLoaderHelper: for 
>>>>>>> Windows, for Unix and for MacOS.
>>>>>>> We have to either duplicate code in Unix and MacOS realizations, 
>>>>>>> or introduce another Helper class for initializing paths only, 
>>>>>>> which would have only two realizations: for Windows and all Unixes.
>>>>>> When I made the comment then I was thinking of a method such as 
>>>>>> allowsQuotedPathElements (or a better name) that returns a 
>>>>>> boolean to indicate if quoting of path elements is allowed or 
>>>>>> not. That would abstract the capability a bit without needing to 
>>>>>> do isWindows checks.
>>>>>>
>>>>> Ah, I see.
>>>>> Though, not needing to check for quotes allows a bit more 
>>>>> efficient implementation, so splitting the code for different 
>>>>> platforms may also make sense.
>>>>>
>>>>> I did it with another helper class in this webrev:
>>>>> http://cr.openjdk.java.net/~igerasim/8067951/3/webrev/
>>>>>
>>>>> Sincerely yours,
>>>>> Ivan
>>>>>
>>>>>
>>>>>> -Alan.
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>
>
>