On Fri, 2 Oct 2020 11:28:49 GMT, Michael McMahon <micha...@openjdk.org> wrote:
>> Continuing this review as a PR on github with the comments below >> incorporated. I expect there will be a few more >> iterations before integrating. >> On 06/09/2020 19:47, Alan Bateman wrote: >>> On 26/08/2020 15:24, Michael McMahon wrote: >>>> >>>> As I mentioned the other day, I wasn't able to use the suggested method on >>>> Windows which returns an absolute path. So, >>>> I added a method to WindowsPath which returns the path in the expected >>>> UTF-8 encoding as a byte[]. Let me know what you >>>> think of that. >>>> >>>> There is a fair bit of other refactoring and simplification done also. >>>> Next version is at: >>>> >>>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/ >>>> >>> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I >>> don't think we should be caching it as the >>> encoding for sun_path is an outlier on Windows. I'm also a bit dubious >>> about encoding a relative path when the resolved >>> path (before encoding) is > 247 chars. The documentation on the MS site >>> isn't very completely and I think there are a >>> number points that require clarification to fully understand how this will >>> work with relative, directly relative and >>> drive relative paths. >>> >> >> Maybe it would be better to just use the path returned from toString() and >> do the conversion to UTF-8 externally. That >> would leave WindowsPath unchanged. >>> In the same area, the new PathUtil is a bit inconsistent with the existing >>> provider code. One suggestion is to add a >>> method to AbstractFileSystemProvider instead. That is the base class the >>> platform providers and would be a better place >>> to get the file path in bytes. >>> >> >> Okay, I gave the method a name that is specific to Unix domain sockets >> because this UTF-8 strangeness is not likely to >> be useful by other components. >>> One other comment on the changes to the file system provider it should be >>> okay to change UnixUserPrinipals ad its >>> fromUid and fromGid method to be public. This would mean that the patch >>> shouldn't need to add UnixUserGroup (the main >>> issue is that class is that it means we end up with two classes with static >>> factory methods doing the same thing). >> >> Okay, that does simplify it a bit. >> >> Thanks, >> Michael. >> >>> -Alan. >>> >>> >>> >>> >>> >>> > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > unixdomainchannels: error in the last commit in > make/modules/java.base/Copy.gmk make/modules/java.base/Copy.gmk line 190: > 188: NET_PROPERTIES_SRC += > $(TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS_TYPE)/conf/net.properties.$(OPENJDK_TARGET_OS_TYPE) > 189: > 190: NET_PROPERTIES_SRC_LIST := $(NET_PROPERTIES_SRC) This variable seems unnecessary as it's just a copy of NET_PROPERTIES_SRC. I would suggest just adding an S for plural to the original variable. make/modules/java.base/Copy.gmk line 195: > 193: $(call MakeTargetDir) > 194: $(RM) $@ $@.tmp > 195: $(foreach f,$(NET_PROPERTIES_SRC_LIST),$(CAT) $(f) >> $@.tmp;) This can be simplified. Cat takes multiple files as input, so no need for 'foreach'. Also no need to go via a temp file. We have make configured to delete targets if a recipe fails, so the tmp dance isn't needed. (I know we still have this pattern all over the place, but we are trying to quit the practice) ------------- PR: https://git.openjdk.java.net/jdk/pull/52
