[SCM] Samba Shared Repository - branch v3-3-test updated - release-3-2-0pre2-2377-g4221937

[Michael Adam]

Hi Jeremy,

Jeremy Allison wrote:
> commit 4221937b68e2414295279b27c5f12a80f826ed4b
> Author: Jeremy Allison <jra at samba.org>
> Date:   Fri May 9 11:14:45 2008 -0700
> 
>     Remove a couple of uses of SMB_VFS_GET_NT_ACL(), use
>     SMB_VFS_FGET_NT_ACL instead. I'd like to ultimately
>     remove SMB_VFS_GET_NT_ACL.
>     Jeremy.

Why?

I had some months ago put some effort in separating
the posix implementations of get_net_acl and fget_nt_acl,
removing the fsp part from the common code portion
and removing the fsp parameter from the SMB_VFS_GET_NT_ACL.

At that time you liked that, and I actually had the plan to
do it for set_nt_acl_too ... :-) I'd like to understand your
motivation in removing the non-F-variants:

> -----------------------------------------------------------------------
> 
> diff --git a/source/rpc_server/srv_srvsvc_nt.c b/source/rpc_server/srv_srvsvc_nt.c
> index 18c6f4d..947ad46 100644
> --- a/source/rpc_server/srv_srvsvc_nt.c
> +++ b/source/rpc_server/srv_srvsvc_nt.c
> ...
> -	nt_status = SMB_VFS_GET_NT_ACL(conn, filename,
> +	if (!(S_ISDIR(st.st_mode))) {
> +        	nt_status = open_file_ntcreate(conn, NULL, filename, &st,
> +				FILE_READ_ATTRIBUTES,
> +				FILE_SHARE_READ|FILE_SHARE_WRITE,
> +				FILE_OPEN,
> +				0,
> +				FILE_ATTRIBUTE_NORMAL,
> +				0,
> +				NULL, &fsp);
> +
> +	} else {
> +		nt_status = open_directory(conn, NULL, filename, &st,
> +				FILE_READ_ATTRIBUTES,
> +				FILE_SHARE_READ|FILE_SHARE_WRITE,
> +				FILE_OPEN,
> +				0,
> +				FILE_ATTRIBUTE_DIRECTORY,
> +				NULL, &fsp);
> +	}
> +
> +	if (!NT_STATUS_IS_OK(nt_status)) {
> +		DEBUG(3,("_srvsvc_NetGetFileSecurity: can't open %s\n",
> +			filename));
> +		werr = ntstatus_to_werror(nt_status);
> +		goto error_exit;
> +	}
> +
> +	nt_status = SMB_VFS_FGET_NT_ACL(fsp,
>  				       (OWNER_SECURITY_INFORMATION
>  					|GROUP_SECURITY_INFORMATION
>  					|DACL_SECURITY_INFORMATION), &psd);

What is the benefit here?

Look at posix_fget_nt_acl(): in case of a directory, it calls
posix_get_nt_acl(). Otherwise an additional stat call which
would use the stat cache is replaces by an open_fil_ntcreate().

So it cannot possibly be performance?

> diff --git a/source/smbd/nttrans.c b/source/smbd/nttrans.c
> index 362823d..bd34b5a 100644
> --- a/source/smbd/nttrans.c
> +++ b/source/smbd/nttrans.c
> @@ -1612,14 +1612,8 @@ static void call_nt_transact_query_security_desc(connection_struct *conn,
>  	if (!lp_nt_acl_support(SNUM(conn))) {
>  		status = get_null_nt_acl(talloc_tos(), &psd);
>  	} else {
> -		if (fsp->fh->fd != -1) {
> -			status = SMB_VFS_FGET_NT_ACL(
> -				fsp, security_info_wanted, &psd);
> -		}
> -		else {
> -			status = SMB_VFS_GET_NT_ACL(
> -				conn, fsp->fsp_name, security_info_wanted, &psd);
> -		}
> +		status = SMB_VFS_FGET_NT_ACL(
> +			fsp, security_info_wanted, &psd);
>  	}
>  
>  	if (!NT_STATUS_IS_OK(status)) {

For the posix implementation, this just removes a double check:
beginning of posix_fget_nt_acl(), this will hit the following code.

...
if (fsp->is_directory ||  fsp->fh->fd == -1) {
	return posix_get_nt_acl(fsp->conn, fsp->fsp_name,
				security_info, ppdesc);
}
...

For other implementations, may it not even fail if fd == -1 ?

I'd really like to understand your motivation in removing the
non-F-variants of the GET/SET_NT_ACL calls!

Cheers - Michael

-- 
Michael Adam <ma at sernet.de>  <obnox at samba.org>
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.SerNet.DE, mailto: Info @ SerNet.DE
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20080509/5e0c28c9/attachment.bin