On Wed, 19 Jun 2024 01:50:30 GMT, Alex Menkov <amen...@openjdk.org> wrote:
> The change fixes a bug in Attach API implementation on Windows when
> argument(s) are longer than 1023 bytes
>
> testing: test/hotspot/jtreg/serviceability/attach,
> test/jdk/com/sun/tools/attach on Oracle supported platforms
The fix looks good. Posted some nits and questions.
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.
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?
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.
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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/19780#pullrequestreview-2173973061
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1675379386
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676279668
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676373808
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676377248
