On Fri, 12 Jul 2024 06:27:46 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c line 57:
>
>> 55: doPrivilegedOpenProcess(DWORD dwDesiredAccess, BOOL bInheritHandle,
>> DWORD dwProcessId);
>> 56:
>> 57: /* Convert jstring to C string, returns JNI_FALSE if the string has been
>> truncated. */
>
> Nit: It is better to have it either:
> "Convert jstring to C string, return JNI_FALSE ..."
> or
> "Converts jstring to C string, returns JNI_FALSE"
>
> The same applies to the comment at line 621.
Fixed
> test/hotspot/jtreg/serviceability/attach/LongArgTest.java line 94:
>
>> 92: HotSpotVirtualMachine vm =
>> (HotSpotVirtualMachine)VirtualMachine.attach(String.valueOf(app.getPid()));
>> 93:
>> 94: if (setFlag(vm)) {
>
> Q: Should the test fail if there an `IOException` was caught in the
> `setFlag()` call?
Currently Attach operations have restriction for argument size, so setFlag() is
expected to fail for long values.
This fix adds AttachOperationFailedException for the case on windows,
linux/bsd/aix implementations throw generic IOException.
(There is [JDK-8334168](https://bugs.openjdk.org/browse/JDK-8334168) to throw
AttachOperationFailedException instead of IOException if an argument is too
long on other platforms)
> test/hotspot/jtreg/serviceability/attach/LongArgTest.java line 101:
>
>> 99: + (actualValue == null
>> 100: ? "null"
>> 101: : ("size is " +
>> actualValue.length() + " instead of " + flagValue.length()));
>
> Q: What if value is different but size is the same?
> I understand the probability is very low but this is still confusing.
I updated the code to check if the value was truncated
> test/hotspot/jtreg/serviceability/attach/LongArgTest.java line 126:
>
>> 124: ex.printStackTrace(System.out);
>> 125: return false;
>> 126: }
>
> Q: Is it always the case there is no need to close the `BufferedReader` in
> this context? Just want to understand.
We can get exception here only if vm.setFlag() throws it. In the case
BufferedReader is not yet constructed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676454684
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676437197
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676454322
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676454488
