Looks good to me in its current form, thanks. On Fri, Mar 27, 2020 at 8:02 AM Toshio 5 Nakamura <toshi...@jp.ibm.com> wrote:
> Hi Thomas, > > Yes, I tested it locally with small buffer size, 50 for example. But, it's > hard to create a jtreg test. > > I confirmed ERROR_INSUFFICIENT_BUFFER returned from GetLastError(), if the > provided buffer was not enough. > Furthermore, "lpdwSize" parameter didn't set a new value in that case. > I hope the manual has more specific description... > > Best regards, > Toshio Nakamura > > > ----- Original message ----- > > Hi Toshio, > > did you test the malloc path? Since > https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-queryfullprocessimagenamew#return-value > does > not mention ERROR_INSUFFICIENT_BUFFER as return code, would like to see > this working. It does however mention that it returns the number of > returned characters, so if the return code does not work out we may use > that. > > If you tested the truncation path and it works I am fine with the change > as it is. > > Cheers, Thomas > > > On Fri, Mar 27, 2020 at 6:08 AM Toshio 5 Nakamura <toshi...@jp.ibm.com> > 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: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.01 > > Best regards, > Toshio Nakamura > > Yasumasa Suenaga <suen...@oss.nttdata.com> 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://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-queryfullprocessimagenamew > > > >>>> > > >>>>>> > > >>>>>> 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://bugs.openjdk.java.net/browse/JDK-8232846 > > > >>>> > > >>>>>>> Webrev: > > >>>> > http://cr.openjdk.java.net/~tnakamura/8232846/webrev.00 > > > >>>> > > >>>>>>> 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 > > >>>>>>> > > >>> > > > > > > > > >
