Hi Toshio,
Looks good,
I can sponsor it.
Thanks, Roger
On 3/27/20 6:46 AM, Toshio 5 Nakamura wrote:
Thank you for review, Suenaga-san, Thomas.
(My mail in html format was blocked by the list. Sorry for inconvenience.)
The latest webrev is 02:
http://cr.openjdk.java.net/~tnakamura/8232846/webrev.02/
Thanks,
Toshio
From: Yasumasa Suenaga <[email protected]>
Ok, your change looks good.
Thanks,
Yasumasa
On 2020/03/27 18:10, Toshio 5 Nakamura wrote:
Hi Suenaga-san,
Thank you for the comment. I updated the limit to 32768.
webrev.02:
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.02&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=PIzjJwY4DhVqnxGp-4WusnNS4MlBy9PEGoCiCE9Hnaw&e=
Well, I believe the new logic works as same as the current one.
I added NULL initialization to commandObj, and the value will be
changed only if QueryFullProcessImageNameW() was success.
Then, CHECK_NULL(commandObj) works as "return" if commandObj is NULL.
src/java.base/share/native/libjava/jni_util.h
> #define CHECK_NULL(x) \
> do { \
> if ((x) == NULL) { \
> return; \
> } \
> } while (0) \
I hope this solves your concern.
Best regards,
Toshio Nakamura
----- Original message -----
Nakamura-san,
I think location of CHECK_NULL and SetObjectField() are incorrect.
Currently they are called when QueryFullProcessImageName() succeed
only.
In addition, you need to allocate 32768 chars (32767 + 1 ('\0'))
via malloc.
I understand "32767" is max length, not includes null-terminator.
Thanks,
Yasumasa
On 2020/03/27 14:06, Toshio 5 Nakamura wrote:
>
> Hi Roger, Suenaga-san,
>
> Thank you for your comments and discussion for the issue.
> I've updated webrev. Could you review it again?
> tier1 tests passed.
>
> webrev.01:
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.01&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=sSZ6mylujPpWlHliOmnz9ukEJoaJrOnLJWuTptntVj8&e=
>
> Best regards,
> Toshio Nakamura
>
> Yasumasa Suenaga <[email protected]> wrote on 2020/
03/27 09:10:15:
>
>> On 2020/03/27 9:01, Roger Riggs wrote:
>>> Hi,
>>>
>>> Please don't throw an exception.
>>> It would abort being able to provide any information.
>>> And who would expect or handle such an exception?
>>>
>>> Degrading the info or omitting it provides better service
overall.
>>> There is no point to throwing an exception.
>>
>> hProcess in QueryFullProcessImageNameW needs
>> PROCESS_QUERY_INFORMATION or PROCESS_QUERY_LIMITED_INFORMATION
>> access right, so I thought it is reasonable to throw runtime
>> exception if the call failed.
>>
>> Anyway, I agree with you if throwing exception is not
comfortable in
>> this case for Java API.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>> Regards, Roger
>>>
>>>
>>> On 3/26/20 7:55 PM, Yasumasa Suenaga wrote:
>>>> On 2020/03/27 0:35, Roger Riggs wrote:
>>>>> Hi,
>>>>>
>>>>> Reading further down the reference to the section:
>>>>> "Enable Long Paths in Windows 10, Version 1607, and Later"
>>>>> might suggest the code be more resilient to long paths.
>>>>>
>>>>> The stack allocated buffer (1024) is fine, but I'd suggest
>> adding code to retry in the case
>>>>> of INSUFFICIENT BUFFER with a malloc'd buffer to make it
more
>> robust since the enabling
>>>>> of long paths is environment and application specific.
>>>>
>>>> In addition, it's better to throw an exception if the call
failed
>> due to other error code.
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>>> Thanks, Roger
>>>>>
>>>>>
>>>>>
>>>>> On 3/26/20 10:40 AM, Toshio 5 Nakamura wrote:
>>>>>> Hi Thomas, Suenaga-san, Roger,
>>>>>>
>>>>>> Thank you for reviewing and comments.
>>>>>>
>>>>>> I checked behaviors by a small program and debugger.
>>>>>> If QueryFullProcessImageNameW failed by not enough buffer,
>>>>>> the API didn't change the buffer.
>>>>>> It just set ERROR_INSUFFICIENT_BUFFER(0x7a) to LastError.
>>>>>>
>>>>>> The buffer size becomes 1024 characters (2048 bytes) by
this patch.
>>>>>> Should it use bigger size with malloc? 32,767 characters
can be limit
> [2].
>>>>>> I feel 1024 is reasonable value for command's location.
>>>>>>
>>>>>> [2]
https://urldefense.com/v3/__https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file*maximum-path-length-limitation__;Iw!!GqivPVa7Brio!PgzCcUQOjCNpcVeetU_drS4DFFVLaj0ceJBvipX7iDc_RlPtcEkdH8AzelGtzqXS
$
>>>>>> Best regards,
>>>>>> Toshio Nakamura
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> If the call fails, the command field in the info will not
be set (and
>>>>>>> therefore null).
>>>>>>>
>>>>>>> I agree that a length of 512 (characters) is probablytoo
small.
>>>>>>>
>>>>>>> Roger
>>>>>>>
>>>>>>>
>>>>>>> On 3/26/20 6:37 AM, Yasumasa Suenaga wrote:
>>>>>>>> Hi Nakamura-san,
>>>>>>>>
>>>>>>>> Your change almost looks good, but I have one question.
>>>>>>>> Length of exeName (1024) is enough?
>>>>>>>>
>>>>>>>> According to Microsoft Doc [1], exeName might not be
null-terminated
>>>>>>>> if it failed.
>>>>>>>> I concern buffer overrun might occur when
QueryFullProcessImageNameW
>>>>>>>> failed.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> [1]
>>>>>>
>
https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.microsoft.com_en-2Dus_windows_win32_api_winbase_nf-2Dwinbase-2Dqueryfullprocessimagenamew&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=V6oP8SJvlxIc2iOxIOAcY6_mznTaxta2tyCV2iVzD3I&e=
>
>>>>>>
>>>>>>>>
>>>>>>>> On 2020/03/26 17:58, Toshio 5 Nakamura wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>> Could you review this change? Additionally, I'd like to
ask a sponsor
>>>>>>>>> for it, since I'm not a committer.
>>>>>>>>>
>>>>>>>>> Bug:
>>>>>>
>
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8232846&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=8iQnOHT0GkoAZsYE39mDmx8036dUvvDbcj4tjCaPyps&e=
>
>>>>>>
>>>>>>>>> Webrev:
>>>>>>
>
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.00&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=wzpmzDaZdDfLKNqZprr1X6d_EdWvLpBWS6fMzVNeu3w&s=0avs_hvCJdUYCP8WPu8Ttlo4sLwbwuFs2wWV0Szc9gg&e=
>>>>>>>>> Under Windows environments, ProcessHandle.Info.command
shows question marks
>>>>>>>>> if the command has non-English characters. I'd like to
change the
>>>>>>>>> underlying API to Unicode version.
>>>>>>>>> Tier1 tests passed.
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Toshio Nakamura
>>>>>>>>>
