[PATCH] vfs_streams_xattr: Do not attempt to write empty attributetwice

[Christof Schmitt]

On Thu, Jun 13, 2013 at 03:56:23PM -0400, Scott Lovenberg wrote:
> On Thu, Jun 13, 2013 at 3:50 PM, Volker Lendecke
> <Volker.Lendecke at sernet.de> wrote:
> > That is probably true. But I think that this should be made
> > obvious with braces. I find the operator precedence of || vs
> > && less than obvious, and a bit of redundancy does not hurt
> > here I guess.
> >
> > Looking at that whole routine, I think the general logic
> > could be simplified quite a bit. (I know, much of that stuff
> > is mine...)
> >
> > Volker
> 
> Yeah, I didn't even consider operator order until the second glance.
> The binary will probably be the same either way and Volker's advise
> makes the code much more readable.

Attached is an updated version of the patch with the braces added. My
preference would be to fix the problem before restructuring the code,
but i can still take a look.

Volker, how would you simplify this function?

-- 
Christof Schmitt || IBM || SONAS System Development || Tucson, AZ
christof.schmitt at us.ibm.com  ||  +1-520-799-2469  (T/L: 321-2469)
-------------- next part --------------
>From 56dc5649d631dfab55c893bc2d4b99e61d13b691 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <christof.schmitt at us.ibm.com>
Date: Wed, 12 Jun 2013 14:55:15 -0700
Subject: [PATCH] vfs_streams_xattr: Do not attempt to write empty attribute twice

The create disposition FILE_OVERWRITE_IF is mapped to the flags
O_CREAT|O_TRUNC. In vfs_streams_xattr, this triggers two calls to
SMB_VFS_SETXATTR. The second can fail if O_EXCL is also set, resulting
in an unnecessary error.

Merge the identical code to handle O_CREAT and O_TRUNC to avoid setting
an empty attribute twice. Also add the flags parameter to the debug
message.

Signed-off-by: Christof Schmitt <christof.schmitt at us.ibm.com>
---
 source3/modules/vfs_streams_xattr.c |   42 +++++++++-------------------------
 1 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/source3/modules/vfs_streams_xattr.c b/source3/modules/vfs_streams_xattr.c
index 82e2dd8..11a3cc6 100644
--- a/source3/modules/vfs_streams_xattr.c
+++ b/source3/modules/vfs_streams_xattr.c
@@ -364,8 +364,8 @@ static int streams_xattr_open(vfs_handle_struct *handle,
 	int baseflags;
 	int hostfd = -1;
 
-	DEBUG(10, ("streams_xattr_open called for %s\n",
-		   smb_fname_str_dbg(smb_fname)));
+	DEBUG(10, ("streams_xattr_open called for %s with flags 0x%x\n",
+		   smb_fname_str_dbg(smb_fname), flags));
 
 	if (!is_ntfs_stream_smb_fname(smb_fname)) {
 		return SMB_VFS_NEXT_OPEN(handle, smb_fname, fsp, flags, mode);
@@ -447,40 +447,20 @@ static int streams_xattr_open(vfs_handle_struct *handle,
 		goto fail;
 	}
 
-	if (!NT_STATUS_IS_OK(status)) {
+	if ((!NT_STATUS_IS_OK(status) && (flags & O_CREAT)) ||
+	    (flags & O_TRUNC)) {
 		/*
-		 * The attribute does not exist
+		 * The attribute does not exist or needs to be truncated
 		 */
 
-                if (flags & O_CREAT) {
-			/*
-			 * Darn, xattrs need at least 1 byte
-			 */
-                        char null = '\0';
+		/*
+		 * Darn, xattrs need at least 1 byte
+		 */
+		char null = '\0';
 
-			DEBUG(10, ("creating attribute %s on file %s\n",
-				   xattr_name, smb_fname->base_name));
+		DEBUG(10, ("creating or truncating attribute %s on file %s\n",
+			   xattr_name, smb_fname->base_name));
 
-			if (fsp->base_fsp->fh->fd != -1) {
-                        	if (SMB_VFS_FSETXATTR(
-					fsp->base_fsp, xattr_name,
-					&null, sizeof(null),
-					flags & O_EXCL ? XATTR_CREATE : 0) == -1) {
-					goto fail;
-				}
-			} else {
-	                        if (SMB_VFS_SETXATTR(
-					handle->conn, smb_fname->base_name,
-					xattr_name, &null, sizeof(null),
-					flags & O_EXCL ? XATTR_CREATE : 0) == -1) {
-					goto fail;
-				}
-			}
-		}
-	}
-
-	if (flags & O_TRUNC) {
-		char null = '\0';
 		if (fsp->base_fsp->fh->fd != -1) {
 			if (SMB_VFS_FSETXATTR(
 					fsp->base_fsp, xattr_name,
-- 
1.7.1