Hi Alan, thanks for diving into this and giving a comprehensive summary.
I've just read it in detail now - and as always, it contains some very good findings and suggestions. I'll try to work these in and send an update in the next days. Best regards Christoph > -----Original Message----- > From: Alan Bateman <alan.bate...@oracle.com> > Sent: Montag, 25. Februar 2019 09:31 > To: Langer, Christoph <christoph.lan...@sap.com> > Cc: nio-dev <nio-...@openjdk.java.net>; Java Core Libs <core-libs- > d...@openjdk.java.net> > Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions > > On 21/02/2019 15:04, Langer, Christoph wrote: > > Hi Alan, > > > > here is the next > iteration:http://cr.openjdk.java.net/~clanger/webrevs/8213031.7/ > > > > I focused on your comments regarding implementation details: > > > > : > > Are there other major implementation points left? If not I guess we should > start refining the documentation... > > I think it would be useful to write down a summary of the proposal as > there are several detailed points that we've been discussing in this > thread that reviewers will need to understand the proposal before diving > into the implementation/patch. This might also help getting the javadoc > aligned, and eventually the CSR. Here's where I think we are: > > 1. By default, a zip file system will have no support for the "posix" > and "owner" views of file attributes, this means the following will fail > with UOE: > > PosixFileAttributes attrs = Files.readAttributes(file, > PosixFileAttributes.class); > > and the zip file system's supportedFileAttributView method will not > include "posix". > > 2. Creating a zip file system with configuration parameter XXX set to > "true" will create the zip file system that supports > PosixFileAttributeView (and FileOwnerAttributeView). The current patch > names the configuration parameter "posix". That name might be misleading > as a configuration parameter as it conveys more than it actually does. > Something like "enablePosixFileAttributes" clear conveys that it enables > support for POSIX file attribute but might be a better wordy. > > The value in the configuration map is documented as Boolean but I think > we are allowing it to be the string representation of a Boolean, meaning > it can be "true" or "false" like we have with the "create" parameter. > > 3. The map of configurations parameters can optionally include the > parameters "defaultOwner", "defaultGroup", and "defaultPermissions" with > the default owner/group/permissions to return. These names seem okay to > me. > > For the owner/group, the parameter type can be > UserPrincipal/GroupPrincipal or strings. If the value is a String then > we need to decide if there is any validation. If I read the current > patch correctly then it doesn't do any validation - that might be okay > but minimally maybe we should disallow the empty string. > > If the defaultXXX parameters aren't present then the zip file > owner/group would be a sensible default. If the zip file is on a file > store that supports FileOwnerAttributeView then we've got the owner. If > the file store supports PosixFileAttributeView then we have a group > owner. If PosixFileAttributeView is not supported then using the owner > as the group is okay. I see the current patch as a fallback to fallback > to "<zipfs_default>" but I'd prefer not have that because it will be > very confusing to diagnose if there are issues. > > For defaultPermissions, the value is a Set<PosixFilePermissions> or the > string representation. In the implementation, initPermissions can be > changed to use PosixFilePermissions.fromString as it does the > translation that we need. > > 4. As an alternative to the type safe access that PosixFileAttributeView > provides, the proposal is to document the "zip" view of file attributes. > Initially, it will admit to one file attribute for the permissions. The > current patch uses the name "storedPermissions" but it might be simpler > to use "permissions" so that "zip:permissions" and "posix:permissions" > are the same when the zip entry has permissions. With this support you > can write code like this: > Set<PosixFilePermission> perms = (Set<PosixFilePermission>) > Files.getAttribute(file, "zip:permissions"); > > The cast is ugly of course but that's the consequence of not exposing > ZipFileAttributes in the API so there is no type token to specify on the > RHS. > > Documenting the "zip" view brings up a few issues, e.g. will > Files.readAttributes(file, "zip:*") reveal more than "permissions". For > this API it is okay for the map to attributes beyond the specified list. > > 5. There will be no support for specifying as POSIX FileAttribute to > createFile or createDirectory to atomically set the permissions when > creating a new file/directory, UOE will thrown as is is now. > > Is this a reasonable summary? > > -Alan
