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: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.01 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 belimit [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 probably too 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=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ&s=oc_KTGmJUjsfnLf3tjWr9DO1dwWp1TqIZgd2EiHVW14&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=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ&s=pn_uS0DojBk6-R6R5QwtYFJvJpaabhia-eiOtIrJO9Q&e= > >>>> > >>>>>>> Webrev: > >>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Etnakamura_8232846_webrev.00&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=EVbFABcgo-X99_TGI2-qsMtyulHUruf8lAzMlVpVRqw&m=noFyLWDG1dMjO5TlaqM_zvW_f-8fvveS2EoZylT5DMQ&s=NylYK0Q3nFFZRVIaeUna6iaClaJm1CWJ2Zkwy-ysvv4&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 > >>>>>>> > >>> > > >
