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
Thu Aug 15 12:42:51 UTC 2019
Hi Adam, David noticed that my code review replies only went to the
mailing list. So here it is again.
Thanks,
Coleen
On 8/13/19 5:29 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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