RFR: JDK-8227021: VM fails if any sun.boot.library.path paths are longer than JVM_MAXPATHLEN
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Aug 13 21:29:12 UTC 2019
On 8/13/19 11:48 AM, Adam Farley8 wrote:
> Hi David,
>
> Since we have two positive reviews (yours and Serguei's), could you
> sponsor the change please?
If you get two positive reviews, but then one with comments, you need to
also answer that third reviews. I had some comments on your change,
which I'll repeat here:
http://cr.openjdk.java.net/~afarley/8227021.2/webrev/src/hotspot/share/runtime/os.cpp.frames.html
This is code is unnecessarily hard to read and understand:
1329 char** os::split_path(const char* path, size_t* n) {
1330 size_t pathmod = (size_t)0;
1331 if (n != NULL) {
1332 pathmod = *n;
1333 }
1334 *n = (size_t)0;
It appears that 'n' is both an input and output parameter of this
function. Can you just make it another input parameter with some
descriptive name. Is it the length of the library name that you're
looking for? "pathmod" is huh?
Then you don't have to check if someone has passed NULL. There's only
one caller to this from what I see.
1352 char** opath = (char**) NEW_C_HEAP_ARRAY(char*, count, mtInternal);
1353 if (opath == NULL) {
1354 return NULL;
1355 }
Not your change but this calls vm_exit_out_of_memory() if
NEW_C_HEAP_ARRAY fails. It doesn't return NULL so you don't have to check.
1375 char* s = (char*)NEW_C_HEAP_ARRAY(char, len + 1, mtInternal);
Here you want to use NEW_C_HEAP_ARRAY_RETURN_NULL so you can check for
null and free memory.
The rest seems okay.
thanks,
Coleen
>
> Best Regards
>
> Adam Farley
> IBM Runtimes
>
>
> David Holmes <david.holmes at oracle.com> wrote on 12/08/2019 23:32:14:
>
>> From: David Holmes <david.holmes at oracle.com>
>> To: Adam Farley8 <adam.farley at uk.ibm.com>
>> Cc: hotspot-dev at openjdk.java.net, "serguei.spitsyn at oracle.com"
>> <serguei.spitsyn at oracle.com>
>> Date: 12/08/2019 23:32
>> Subject: Re: RFR: JDK-8227021: VM fails if any sun.boot.library.path
>> paths are longer than JVM_MAXPATHLEN
>>
>> Looks fine.
>>
>> Thanks,
>> David
>>
>> On 13/08/2019 1:19 am, Adam Farley8 wrote:
>>> Hi David,
>>>
>>> Changes made as requested:
>>> https://urldefense.proofpoint.com/v2/url?
>> u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.4_webrev&d=DwID-
>> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>>
> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=mZM8IR4e_zmrfcsF3XJjoGEMrZb4WtEL7Y6Ugd6Naqg&s=sZ2UKqqUIq0El-
>> RsqYz6jmTh4Q2UghwdrQX6of8Lw0E&e=
>>> Best Regards
>>>
>>> Adam Farley
>>> IBM Runtimes
>>>
>>>
>>> David Holmes <david.holmes at oracle.com> wrote on 12/08/2019 04:55:36:
>>>
>>>> From: David Holmes <david.holmes at oracle.com>
>>>> To: Adam Farley8 <adam.farley at uk.ibm.com>,
>>>> "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>>>> Cc: hotspot-dev at openjdk.java.net
>>>> Date: 12/08/2019 04:56
>>>> Subject: Re: RFR: JDK-8227021: VM fails if any sun.boot.library.path
>>>> paths are longer than JVM_MAXPATHLEN
>>>>
>>>> Hi Adam,
>>>>
>>>> On 10/08/2019 2:47 am, Adam Farley8 wrote:
>>>>> Hi Serguei, David,
>>>>>
>>>>> My turn to apologise for the delay. :)
>>>>>
>>>>> I've modified the code as per Serguei's request. Take a look and
> let me
>>>>> know if this is the sort of thing you were thinking of.
>>>>>
>>>>> Webrev: https://urldefense.proofpoint.com/v2/url?
>>>> u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.3_webrev_&d=DwID-
>>>> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>>>> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=8Wa8Zdfmvn-
>>>>
> yvzvCAhOyJ_etFblRA4vmLGbKF4aW8PY&s=L19aeFXoR9JIO62QRPFzZObIU8RbhpCtXSvUibD2ISk&e=
>>>> I'd prefer to see the helper just as a file static function rather
> than
>>>> adding it to the os class.
>>>>
>>>> + * supplied array of arrays of chars (a), where n
>>>>
>>>> I assume (a) is meant to refer to the parameter, but you actually
> called
>>>> it arrayarray. I think "a" or "arr" would suffice.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Best Regards
>>>>>
>>>>> Adam Farley
>>>>> IBM Runtimes
>>>>>
>>>>>
>>>>> "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote on
>>>>> 31/07/2019 17:18:05:
>>>>>
>>>>>> From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>>>>>> To: Adam Farley8 <adam.farley at uk.ibm.com>, David Holmes
>>>>>> <david.holmes at oracle.com>
>>>>>> Cc: serviceability-dev <serviceability-dev at openjdk.java.net>,
>>>>>> hotspot-dev at openjdk.java.net
>>>>>> Date: 31/07/2019 17:18
>>>>>> Subject: Re: RFR: JDK-8227021: VM fails if any
> sun.boot.library.path
>>>>>> paths are longer than JVM_MAXPATHLEN
>>>>>>
>>>>>> Hi Adam,
>>>>>>
>>>>>> It looks Okay to me.
>>>>>>
>>>>>> A couple of minor comments.
>>>>>>
>>>>>> https://urldefense.proofpoint.com/v2/url?
>>>> u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.
>>>> 2_webrev_src_hotspot_&d=DwID-g&c=jf_iaSHvJObTbx-
>>>> siA1ZOg&r=P5m8KWUXJf-CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=8Wa8Zdfmvn-
>>>>
> yvzvCAhOyJ_etFblRA4vmLGbKF4aW8PY&s=NL6tYuwwDod3DSmj-1ztxAywpO8L52HEyO0wvTR05bs&e=
>>>>>> share/runtime/os.cpp.frames.html
>>>>>> 1362 //release allocated storage before exiting the vm
>>>>>> 1363 while (i > 0) {
>>>>>> 1364 i--;
>>>>>> 1365 if (opath[i] != NULL) {
>>>>>> 1366 FREE_C_HEAP_ARRAY(char, opath[i]);
>>>>>> 1367 }
>>>>>> 1368 }
>>>>>> 1369 FREE_C_HEAP_ARRAY(char*, opath);
>>>>>>
>>>>>> 1377 //release allocated storage before returning null
>>>>>> 1378 while (i > 0) {
>>>>>> 1379 i--;
>>>>>> 1380 if (opath[i] != NULL) {
>>>>>> 1381 FREE_C_HEAP_ARRAY(char, opath[i]);
>>>>>> 1382 }
>>>>>> 1383 }
>>>>>> 1384 FREE_C_HEAP_ARRAY(char*, opath);
>>>>>>
>>>>>> This duplicated fragments is worth to refactor to a function.
>>>>>> Also a space is missed at the beginning of the comment.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 7/31/19 02:01, Adam Farley8 wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Reviewers requested for the change below.
>>>>>>
>>>>>> @David - Agreed. Would you be prepared to sponsor the change?
>>>>>>
>>>>>> Bug: https://urldefense.proofpoint.com/v2/url?
>>>> u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8227021&d=DwID-
>>>> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>>>> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=8Wa8Zdfmvn-
>>>>
> yvzvCAhOyJ_etFblRA4vmLGbKF4aW8PY&s=xykJ0KLy9AKWO8zmC0amfR7xxUsvyKEjlf3y7WWOqvE&e=
>>>>>> Webrev: https://urldefense.proofpoint.com/v2/url?
>>>> u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.2_webrev_&d=DwID-
>>>> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>>>> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=8Wa8Zdfmvn-
>>>>
> yvzvCAhOyJ_etFblRA4vmLGbKF4aW8PY&s=NvIza4VVWG3CiDhmQVmXsghH_4h_c5mFJbHwkCUcut0&e=
>>>>>> Best Regards
>>>>>>
>>>>>> Adam Farley
>>>>>> IBM Runtimes
>>>>>>
>>>>>> P.S. Remembered to add the links this time. :)
>>>>>>
>>>>>>
>>>>>> David Holmes <david.holmes at oracle.com> wrote on 30/07/2019
> 03:37:53:
>>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>>> To: Adam Farley8 <adam.farley at uk.ibm.com>
>>>>>>> Cc: hotspot-dev at openjdk.java.net, serviceability-dev
>>>>>>> <serviceability-dev at openjdk.java.net>
>>>>>>> Date: 30/07/2019 03:38
>>>>>>> Subject: Re: RFR: JDK-8227021: VM fails if any
> sun.boot.library.path
>>>>>>> paths are longer than JVM_MAXPATHLEN
>>>>>>>
>>>>>>> Hi Adam,
>>>>>>>
>>>>>>> On 25/07/2019 3:57 am, Adam Farley8 wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> Welcome back. :)
>>>>>>> Thanks. Sorry for the delay in getting back to this.
>>>>>>>
>>>>>>> I like .v2 as it is much simpler (notwithstanding freeing
>> the already
>>>>>>> allocated arrays adds some complexity - thanks for fixing that).
>>>>>>>
>>>>>>> I'm still not sure we can't optimise things better for
> unchangeable
>>>>>>> properties like the boot libary path, but that's another RFE.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>> Unless stated otherwise above:
>>>>>> IBM United Kingdom Limited - Registered in England and Wales with
>>>>>> number 741598.
>>>>>> Registered office: PO Box 41, North Harbour, Portsmouth,
>> Hampshire PO6 3AU
>>>>> Unless stated otherwise above:
>>>>> IBM United Kingdom Limited - Registered in England and Wales with
> number
>>>>> 741598.
>>>>> Registered office: PO Box 41, North Harbour, Portsmouth,
>> Hampshire PO6 3AU
>>> Unless stated otherwise above:
>>> IBM United Kingdom Limited - Registered in England and Wales with
> number
>>> 741598.
>>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6
> 3AU
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>
More information about the hotspot-dev
mailing list