What's wrong with this piece of code from source3/modules/vfs_acl_xattr.c around like 126 or so?

[Michael Adam]

Hi Richard,

I guess the point that the code is trying to make is that
it wants to protect the original errno from being overwritten
by intermediate actions like unbecome_root, debugging, etc.
unbecome_root does quite a lot of operations..

To do that consequently we could change that according
to the attached patch. Does that make it more clear
and maybe more correct? With this patch, the errno is only
reset to the saved value directly before the return.

Cheers - Michael

Richard Sharpe wrote:
> On Mon, May 14, 2012 at 5:47 PM, simo <idra at samba.org> wrote:
> > On Mon, 2012-05-14 at 16:25 -0700, Jeremy Allison wrote:
> >> On Mon, May 14, 2012 at 02:28:59PM -0700, Richard Sharpe wrote:
> >> >         if (ret) {
> >> >                 errno = saved_errno;
> >> >                 DEBUG(5, ("store_acl_blob_fsp: setting attr failed for file %s"
> >> >                         "with error %s\n",
> >> >                         fsp_str_dbg(fsp),
> >> >                         strerror(errno) ));
> >> >                 return map_nt_error_from_unix(errno);
> >> >         }
> >> >
> >> > Hint. It has to do with saved_errno.
> >>
> >> Oh, you mean the errno = saved_errno should
> >> be *after* the DEBUG statement ?
> >>
> >> Just checking - not sure exactly what I missed here.
> >
> > Why are you assigning errno, and not just using saved_errno directly ?
> 
> Well, that's my question. It's not my code, BTW, although I might have
> run into exactly the same mistake.
> 
> -- 
> Regards,
> Richard Sharpe
> (何以解憂？唯有杜康。--曹操)
-------------- next part --------------
From 944920dec82dea8dee1f4e95dd83a2aa514a12fa Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Tue, 15 May 2012 07:55:02 +0200
Subject: [PATCH] s3:vfs:acl_xattr: fix errno handling in store_acl_blob_fsp()

---
 source3/modules/vfs_acl_xattr.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c
index 5653657..4124f3b 100644
--- a/source3/modules/vfs_acl_xattr.c
+++ b/source3/modules/vfs_acl_xattr.c
@@ -119,12 +119,12 @@ static NTSTATUS store_acl_blob_fsp(vfs_handle_struct *handle,
 	}
 	unbecome_root();
 	if (ret) {
-		errno = saved_errno;
 		DEBUG(5, ("store_acl_blob_fsp: setting attr failed for file %s"
 			"with error %s\n",
 			fsp_str_dbg(fsp),
-			strerror(errno) ));
-		return map_nt_error_from_unix(errno);
+			strerror(saved_errno) ));
+		return map_nt_error_from_unix(saved_errno);
+		errno = saved_errno;
 	}
 	return NT_STATUS_OK;
 }
-- 
1.7.9.5

-------------- 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/pipermail/samba-technical/attachments/20120515/e1d759c5/attachment.pgp>