Hi Alan, Volker, thanks for bringing up these discussion points. I agree that we shall carefully revisit that.
First of all, I'd like to point out that PosixFileAttributeView::readAttributes won't ever throw UOE with the proposed changes to zipfs. It should always return an object of type PosixFileAttributes which will be an incarnation of ZipFileSystem.Entry. The returned PosixFileAttributes(ZipFileSystem.Entry) object now implements 3 additional methods: owner(), group() and permissions(). I can see that UOE should only be thrown if something is not supported by an implementation at all. So it perfectly fits to owner() and group() because this is simply not implemented in zipfs (yet...). As for permissions, I then agree, UOE probably isn't the right thing to do because permissions will now be supported by zipfs. Still, we need to distinguish between the case where no permission information is present vs. an empty set of permissions that was explicitly stored. You brought up IOException as an alternative. But I think this is not really feasible for the following main reason: IOE is no RuntimeException, so it has to be declared for a method when it is thrown. PosixFileAttributes::permissions, however, does not declare IOE so far. And I don't think we can/want to modify the PosixFileAttributes interface for the sake of that zipfs implementation change. I think, we should look into using/returning null for "no permission information present". With that, the point is: What happens if we pass null to another PosixFileAttributeView::setPermissions implementation (or to Files::setPosixFilePermissions, which ends up there). For zipfs, with the proposed changeset, this would work perfectly fine. We translate the null value into (int)-1 for the "posixPerms" field of "Entry" and will then not set permission information in the zip file. But other places in the JDK that implement PosixFileAttributeView need some rework. Those are: src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.Posix src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.PosixFileAttributeViewImpl And we should amend the specs/doc for the following methods to define the handling of the null value as a noop: PosixFileAttributeView::setPermissions PosixFileAttributes::permissions Files::setPosixFilePermissions Files::getPosixFilePermissions In the implementation I can see that we modify the aforementioned places to handle null by translating it to (int)-1 in UnixFileModeAttribute::toUnixMode and then using this value as noop in 'setMode' resp. 'fchmod'. Do you think this is the right way to go? Should we maybe do the spec update of the permission methods in a separate change? Best regards Christoph > -----Original Message----- > From: Alan Bateman <alan.bate...@oracle.com> > Sent: Montag, 7. Januar 2019 21:46 > To: Volker Simonis <volker.simo...@gmail.com> > Cc: Langer, Christoph <christoph.lan...@sap.com>; nio-dev <nio- > d...@openjdk.java.net>; OpenJDK Dev list <security- > d...@openjdk.java.net>; Java Core Libs <core-libs-dev@openjdk.java.net> > Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions > > On 07/01/2019 19:26, Volker Simonis wrote: > > : > > We considered this, but it is problematic because it is perfectly > > valid to have a file with external file attributes where none of the > > Posix attributes is actually set (i.e. an empty set of Posix files > > attributes). This wouldn't be distinguishable from the case where a > > file has no external file attributes. So it seems we have to resort to > > throwing an IOE? > > > Maybe although it would be a bit awkward to deal with. The issues around > this remind me a bit about mounting fat32 file systems on Linux or Unix > systems where the fields in the stat structure are populated with > default values. PosixFileAttributeView::readAttributes is essentially > the equivalent of a stat call. This might be something to look at for > the file owner at least. > > -Alan
