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

[Alan Bateman]

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