[PATCH] smbd: stop disabling "store dos attributes" on-the-fly

[Christof Schmitt]

On Thu, Dec 24, 2015 at 10:51:41AM +0200, Uri Simchoni wrote:
> Hi,
> 
> Review appreciated.
> Thanks,
> Uri.
> 

Hi Uri,

i noticed that the patch is already in master, but please see below for a
comment on the messages. Maybe you could post a follow-on patch to remove the
function name.

Christof

> From b9612056a73813e82f9ceee0aca9018f945c5a9a Mon Sep 17 00:00:00 2001
> From: Uri Simchoni <uri at samba.org>
> Date: Thu, 24 Dec 2015 08:10:11 +0200
> Subject: [PATCH] smbd: do not disable "store dos attributes" on-the-fly
> 
> Smbd would disable "store dos attributes" on-the-fly if the
> attempt to set/get user.DOSATTRIB fails with ENOTSUP or ENOSYS.
> The rationale behind it was that the file system does not support
> extended attributes, so there's no need to fill up the log with
> failure messages.
> 
> However, a "wide symlink" could point to a spot that doesn't support
> extended attributes. Even with the default banned wide links, we
> currenly allow stat'ing those files and follow the symlink, and this
> in turn would disable "store dos attributes" for the whole share.
> The user.DOSATTRIB attribute also stores file creation time,
> so that is also affected.
> 
> Another case where this behavior would turn storage of DOS attributes
> off is that of the ".." entry at the root of the share, if the parent
> folder for the share's root path does not support extended attributes.
> 
> On the other hand, the information on the file system and its support
> of extended attributes is readily available, so the fix for explosion
> of the log should be not to configure "store dos attributes" on
> such a share.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11649
> 
> Signed-off-by: Uri Simchoni <uri at samba.org>
> ---
>  source3/include/proto.h  |  1 -
>  source3/param/loadparm.c | 11 -----------
>  source3/smbd/dosmode.c   | 30 ++++++------------------------
>  3 files changed, 6 insertions(+), 36 deletions(-)
> 
> diff --git a/source3/include/proto.h b/source3/include/proto.h
> index 4cd69fd..cc00a84 100644
> --- a/source3/include/proto.h
> +++ b/source3/include/proto.h
> @@ -1067,7 +1067,6 @@ uint32_t lp_get_spoolss_state( void );
>  struct smb_signing_state;
>  bool lp_use_sendfile(int snum, struct smb_signing_state *signing_state);
>  void set_use_sendfile(int snum, bool val);
> -void set_store_dos_attributes(int snum, bool val);
>  void lp_set_mangling_method(const char *new_method);
>  bool lp_posix_pathnames(void);
>  void lp_set_posix_pathnames(void);
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index 348298d..9f4a2b4 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -4262,17 +4262,6 @@ void set_use_sendfile(int snum, bool val)
>  		sDefault._use_sendfile = val;
>  }
>  
> -/*******************************************************************
> - Turn off storing DOS attributes if this share doesn't support it.
> -********************************************************************/
> -
> -void set_store_dos_attributes(int snum, bool val)
> -{
> -	if (!LP_SNUM_OK(snum))
> -		return;
> -	ServicePtrs[(snum)]->store_dos_attributes = val;
> -}
> -
>  void lp_set_mangling_method(const char *new_method)
>  {
>  	lpcfg_string_set(Globals.ctx, &Globals.mangling_method, new_method);
> diff --git a/source3/smbd/dosmode.c b/source3/smbd/dosmode.c
> index 0996007..942d286 100644
> --- a/source3/smbd/dosmode.c
> +++ b/source3/smbd/dosmode.c
> @@ -279,18 +279,9 @@ static bool get_ea_dos_attribute(connection_struct *conn,
>  				   SAMBA_XATTR_DOS_ATTRIB, attrstr,
>  				   sizeof(attrstr));
>  	if (sizeret == -1) {
> -		if (errno == ENOSYS
> -#if defined(ENOTSUP)
> -			|| errno == ENOTSUP) {
> -#else
> -				) {
> -#endif
> -			DEBUG(1,("get_ea_dos_attribute: Cannot get attribute "
> -				 "from EA on file %s: Error = %s\n",
> -				 smb_fname_str_dbg(smb_fname),
> -				 strerror(errno)));
> -			set_store_dos_attributes(SNUM(conn), False);
> -		}
> +		DBG_INFO("get_ea_dos_attribute: Cannot get attribute "
> +			 "from EA on file %s: Error = %s\n",
> +			 smb_fname_str_dbg(smb_fname), strerror(errno));

Minor comment: DBG_INFO in master already prepends the function name, there is
no need to explicitly add it. You need to add it in the backport.

>  		return False;
>  	}
>  
> @@ -422,18 +413,9 @@ static bool set_ea_dos_attribute(connection_struct *conn,
>  		files_struct *fsp = NULL;
>  
>  		if((errno != EPERM) && (errno != EACCES)) {
> -			if (errno == ENOSYS
> -#if defined(ENOTSUP)
> -				|| errno == ENOTSUP) {
> -#else
> -				) {
> -#endif
> -				DEBUG(1,("set_ea_dos_attributes: Cannot set "
> -					 "attribute EA on file %s: Error = %s\n",
> -					 smb_fname_str_dbg(smb_fname),
> -					 strerror(errno) ));
> -				set_store_dos_attributes(SNUM(conn), False);
> -			}
> +			DBG_INFO("set_ea_dos_attributes: Cannot set "
> +				 "attribute EA on file %s: Error = %s\n",
> +				 smb_fname_str_dbg(smb_fname), strerror(errno));

Same here.

>  			return false;
>  		}
>  
> -- 
> 2.4.3
>