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 > > >>>>>>>>>
