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

[Sandeep Nashikkar]

On Wed, Aug 15, 2018 03:01 AM IST Jeremy Allison wrote 
> OK, this is a really nice patch. A couple of points:

Thanks. That sounds great  to me :)

> 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).
> 

Ok expanded the macro and returned NULL or 0 specifically as applicable. 

> 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.

Ok. Fixed this. 

> 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 ?

Yes, the existing setups may break. In order to keep them intact, I have added the config switch to enable this encoding. 
We can use following parameters to use the changes done by this patch 

nfs4acl_xattr:encoding = xdr40
nfs4acl_xattr:version = 40

Earlier, xdr40 "encoding" type was not present. I added it in this patch. Also, in the last patch, I had changed the "version" to 40 by default which I reverted back to 41 in this patch. 
I would rather suggest to keep the encoding=xdr40 and version=40 as default, since that works directly with NFS 4.0 ACLs. Let me know what you think. 

On a side note, the nfs4acl_* functions do not comply with NFS 4.1 Spec. 

> 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.

That's great !

> 
> 	Jeremy.

Thanks a lot Jeremy for your time and inputs. I have attached the updated patch. 
Please review and let me know if you have any more questions. 

Thanks
Sandeep

***************************Legal Disclaimer***************************
"This communication may contain confidential and privileged material for the
sole use of the intended recipient. Any unauthorized review, use or distribution
by others is strictly prohibited. If you have received the message by mistake,
please advise the sender by reply email and delete the message. Thank you."
**********************************************************************
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: update3-nfs4acl_xattr_patch_for_nfs40_acl_spec.patch.txt
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180816/40d5d84e/update3-nfs4acl_xattr_patch_for_nfs40_acl_spec.patch.txt>