Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Tue, 12 Mar 2024 18:42:48 GMT, Erik Joelsson wrote: >> Chad Rakoczy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Code cleanup > > I'm fine with just using VERSION_FEATURE. I think it's a simple and > straightforward enough simplification/approximation. If we think we need a > more exact version comparison, then I think we should use one of the version > strings instead of arbitrarily picking a set of version variables. @erikj79, are you good with this version? - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1996757094
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with one additional > commit since the last revision: > > Code cleanup I'm fine with just using VERSION_FEATURE. I think it's a simple and straightforward enough simplification/approximation. If we think we need a more exact version comparison, then I think we should use one of the version strings instead of arbitrarily picking a set of version variables. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1992318595
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with one additional > commit since the last revision: > > Code cleanup Thanks for all this feedback! I am going to try and see if we can pass the full version string. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1992241562
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 20:04:48 GMT, Roger Riggs wrote: >> Chad Rakoczy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Code cleanup > > make/modules/java.base/Launcher.gmk line 85: > >> 83: -DVERSION_INTERIM=$(VERSION_INTERIM) \ >> 84: -DVERSION_UPDATE=$(VERSION_UPDATE) \ >> 85: -DVERSION_PATCH=$(VERSION_PATCH), \ > > Using all 4 is way overkill for the problem at hand. Just the > FEATURE_VERSION is sufficient. > We all know better than to make incompatible changes in minor versions let > alone update or patch version. There is already a `$(VERSION_CFLAGS)` variable defined. It will set all those (and some more). Please use it instead. But then, as Roger says, it is probably overkill to *check* anything but the feature version. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1521525658
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with one additional > commit since the last revision: > > Code cleanup make/modules/java.base/Launcher.gmk line 85: > 83: -DVERSION_INTERIM=$(VERSION_INTERIM) \ > 84: -DVERSION_UPDATE=$(VERSION_UPDATE) \ > 85: -DVERSION_PATCH=$(VERSION_PATCH), \ Using all 4 is way overkill for the problem at hand. Just the FEATURE_VERSION is sufficient. We all know better than to make incompatible changes in minor versions let alone update or patch version. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520364358
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 18:36:57 GMT, Aleksey Shipilev wrote: > > If you really want to require an exact match for jspawnhelper, then these 4 > > numbers aren't necessarily enough, but a rather arbitrarily chosen > > approximation. > > I think for our purposes, a version number quadruplet is enough to provide > basic level of safety for jspawnhelper protocol updates across JDK versions, > without version checking code being a safety problem itself. > > Putting in the full version string would raise more questions, at least for > me. For example, are we guaranteed that version string always fits the > argument line? Probably so. Would we get some interesting (Unicode?) symbols > in version string that would break somewhere along the way? Would we need to > dynamically allocate the buffer for arguments, instead of allocating enough > to hold the version _integers_? Would we make a mistake while doing so? Etc. > > All in all, it feels better to silently accept some version mismatches not > captured by the version quadruplet, rather than fail trying to do a perfect > match. Legal characters for the version string are defined in https://openjdk.org/jeps/223 and verified by configure. There is no risk of weird unicode being included. If the full version string is too long, you could consider using `VERSION_SHORT`, which drops the $OPT, and the $BUILD part. I don't think we need to dynamically allocate the buffer. The required buffer length is known at compile time as the length of the expected version string (which we get as a command line macro) +1. If the user supplies more, we don't need to compare the rest of the string as we already know it's not equal. To me, hardcoding of 4 version digits raises more questions. We spend unnecessary time and effort specifically serializing and de-serializing 4 numbers, always maintaining the correct order. To me that's brittle code. If the version elements, or how we use them, were to change in the future, then this code needs to be carefully revisited, whereas if we just used a standard version string, it would continue to work. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989320103
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy wrote:
>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>>
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are
>> the same. Updates test to include check for version. Also tested manually by
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Code cleanup
src/java.base/unix/native/jspawnhelper/jspawnhelper.c line 173:
> 171: if (jdk_feature != VERSION_FEATURE || jdk_interim !=
> VERSION_INTERIM || jdk_update != VERSION_UPDATE || jdk_patch !=
> VERSION_PATCH) {
> 172: fprintf(stderr, "Expected jspawnhelper for Java %d.%d.%d.%d,
> ", jdk_feature, jdk_interim, jdk_update, jdk_patch);
> 173: fprintf(stderr, "but jspawnhelper for Java %d.%d.%d.%d was
> found.\n", VERSION_FEATURE, VERSION_INTERIM, VERSION_UPDATE, VERSION_PATCH);
Minor: It is a bit odd to see that jspawnhelper found its own version odd. I'd
say:
fprintf(stderr, "jspawnhelper version check failed. jspawnhelper version:
%d.%d.%d.%d, JDK version: %d.%d.%d.%d\n",
VERSION_FEATURE, VERSION_INTERIM, VERSION_UPDATE, VERSION_PATCH,
jdk_feature, jdk_interim, jdk_update, jdk_patch);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520296858
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) > > Updates jspawnhelper to check that JDK version and jspawnhelper version are > the same. Updates test to include check for version. Also tested manually by > replacing jspawnhelper with incorrect version to confirm that check works. Chad Rakoczy has updated the pull request incrementally with one additional commit since the last revision: Code cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/18204/files - new: https://git.openjdk.org/jdk/pull/18204/files/cf3c20fb..7bbcc08f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18204=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18204=00-01 Stats: 48 lines in 4 files changed: 16 ins; 24 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18204.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18204/head:pull/18204 PR: https://git.openjdk.org/jdk/pull/18204
