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
