RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

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.Bateman at oracle.com>
> Sent: Montag, 25. Februar 2019 09:31
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: nio-dev <nio-dev at openjdk.java.net>; Java Core Libs <core-libs-
> dev at 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