[PATCH] Fix for XDR Backend of NFS4ACL_XATTR module to get it working with NFS4.0 ACL Spec

[Jeremy Allison]

On Thu, Jul 26, 2018 at 11:23:50AM +0000, Sandeep Nashikkar via samba-technical wrote:
> 
> Please find the updated patch with overflow checks wherever required. Kindly review and let me know if there are any more comments.  

OK, this is a really nice patch. A couple of points:
the style guide for Samba stipulates that we don't
use control-flow macros like:

+#define OVERFLOW_CHECK(val1, val2)                     \
+do {                                                   \
+       if ((val1) + (val2) < (val1) ) {                \
+               DBG_ERR("Integer Overflow error\n");    \
+               return 0;                               \
+       }                                               \
+} while(0);                                            \

so could you expand those out (yeah I know, it can be
a pain but it does ensure that you're using the
correct error code returns - for example, using
OVERFLOW_CHECK inside nfs40acl_alloc() means you're
doing a 'return 0' instead of a 'return NULL'.

Yes I know they're the same in this case, but expanding
them out by hand makes sure you've gotten the error
returns right (or at least can make it clear if there
are cases where error should return -1 and you're
returning 0).

Secondly, the Samba style guide requires that you
dont' mix code and declarations such as:

+                       uint32_t numeric_id;
+                       if (nace40->who.utf8string_len + 1 < nace40->who.utf8string_len) {
+                               DBG_ERR("Integer overflow error while converting NFS4 ACE\n");
+                               continue;
+                       }
+                       char *id = talloc_zero_size(mem_ctx, nace40->who.utf8string_len + 1);
+                       int err;


- the definition for  char *id and int err must go before any
code.

These are the only issues I can see code-wise.

The other query I have is in the nfs4acl_blob_to_smb4() code change.

You've done:

        case NFS4ACL_ENCODING_XDR:
-               status = nfs4acl_xdr_blob_to_smb4(handle, mem_ctx, blob, smb4acl);
+               status = nfs40acl_xdr_blob_to_smb4(handle, mem_ctx, blob, smb4acl);
                break;
        default:
                status = NT_STATUS_INTERNAL_ERROR;
@@ -325,7 +325,7 @@ static bool nfs4acl_smb4acl_set_fn(vfs_handle_struct *handle,
                                                     smb4acl, &blob);
                break;
        case NFS4ACL_ENCODING_XDR:
-               status = nfs4acl_smb4acl_to_xdr_blob(handle, talloc_tos(),
+               status = nfs40acl_smb4acl_to_xdr_blob(handle, talloc_tos(),
                                                     smb4acl, &blob);

The original nfs4acl_xdr_blob_to_smb4() code de-linearizes
a nfs41 ACL, you're changing it to use nfsv40 ACLs by default.

Isn't that going to break any existing setups ? As far
as I can tell you've removed the ability to access the
nfsv41 ACL code, yeah ?

Is there a way you can do this such that existing
users can get access to their nfsv41 ACLs and your
users can get access to their nfsv40 ACLs via a config
switch ?

Or have you already provided that and I'm too stupid
to see how that's done (wouldn't be the first time :-) ?

Thanks for your patience, this looks a great addition
and I'd love to get it upstream once I fully understand
it.

	Jeremy.