[PATCH] vfs_default: Fix passing of errno from async calls

[Jeremy Allison]

On Wed, Aug 23, 2017 at 04:24:34PM -0700, Christof Schmitt wrote:
> On Wed, Aug 23, 2017 at 04:01:57PM -0700, Jeremy Allison wrote:
> > On Wed, Aug 23, 2017 at 03:58:36PM -0700, Christof Schmitt wrote:
> > > On Wed, Aug 23, 2017 at 03:54:58PM -0700, Jeremy Allison wrote:
> > > > On Wed, Aug 23, 2017 at 03:12:26PM -0700, Christof Schmitt via samba-technical wrote:
> > > > > I found this while testing with a system where the actual write() system
> > > > > call returned an error. This led to Samba hanging and never continuing
> > > > > with the result of the write. The attached patch fixes the problem. From
> > > > > what i understand, errno was never passed correctly back to the callers.
> > > > 
> > > > Wow - good catch !!!!
> > > > 
> > > > Can we change the assignment:
> > > > 
> > > >  -     state->err = errno;
> > > >  +     state->vfs_aio_state.error = errno;
> > > > 
> > > > to:
> > > > 
> > > >  -     state->err = errno;
> > > >  +	if (state->ret == -1) {
> > > >  +	     state->vfs_aio_state.error = errno;
> > > >  +	}
> > > > 
> > > > on each of the hunks, as we need to make
> > > > sure that the thread only assigns non-zero
> > > > to state->vfs_aio_state.error in the fail
> > > > case. It's (theoretically) possible that
> > > > the first go around returns -1, errno = EINTR and
> > > > the second go around returns 0, but
> > > > doesn't set errno (leaving it as the
> > > > previous EINTR value). We should only
> > > > set state->vfs_aio_state.error on a
> > > > *known* error return IMHO.
> > > > 
> > > > If you're happy with that RB+.
> > > 
> > > Yes, that makes sense. Let me test with the change and then send the
> > > updated patch.
> > 
> > Bug ID for back-ports:
> > 
> > https://bugzilla.samba.org/show_bug.cgi?id=12983
> 
> Tested again with the failing pwrite() call. The modified patch
> (attached) catches that case as expected. I also added the bugzilla link
> to the commit message.

Perfect - thanks ! RB+ and pushed.

> From d73a513a0e33e234f2f54aa1c443e31501e5ee07 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 23 Aug 2017 14:37:28 -0700
> Subject: [PATCH] vfs_default: Fix passing of errno from async calls
> 
> Current code assigns errno from async pthreadpool calls to the
> vfs_default internal vfswrap_*_state.  The callers of the vfs_*_recv
> functions expect the value from errno in vfs_aio_state.error.
> 
> Correctly assign errno to vfs_aio_state.error and remove the unused
> internal err variable.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12983
> 
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
>  source3/modules/vfs_default.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
> index fbfe654..0a56e45 100644
> --- a/source3/modules/vfs_default.c
> +++ b/source3/modules/vfs_default.c
> @@ -738,7 +738,6 @@ static int vfswrap_init_pool(struct smbd_server_connection *conn)
>  
>  struct vfswrap_pread_state {
>  	ssize_t ret;
> -	int err;
>  	int fd;
>  	void *buf;
>  	size_t count;
> @@ -812,7 +811,9 @@ static void vfs_pread_do(void *private_data)
>  				   state->offset);
>  	} while ((state->ret == -1) && (errno == EINTR));
>  
> -	state->err = errno;
> +	if (state->ret == -1) {
> +		state->vfs_aio_state.error = errno;
> +	}
>  
>  	PROFILE_TIMESTAMP(&end_time);
>  
> @@ -861,7 +862,6 @@ static ssize_t vfswrap_pread_recv(struct tevent_req *req,
>  
>  struct vfswrap_pwrite_state {
>  	ssize_t ret;
> -	int err;
>  	int fd;
>  	const void *buf;
>  	size_t count;
> @@ -935,7 +935,9 @@ static void vfs_pwrite_do(void *private_data)
>  				   state->offset);
>  	} while ((state->ret == -1) && (errno == EINTR));
>  
> -	state->err = errno;
> +	if (state->ret == -1) {
> +		state->vfs_aio_state.error = errno;
> +	}
>  
>  	PROFILE_TIMESTAMP(&end_time);
>  
> @@ -984,7 +986,6 @@ static ssize_t vfswrap_pwrite_recv(struct tevent_req *req,
>  
>  struct vfswrap_fsync_state {
>  	ssize_t ret;
> -	int err;
>  	int fd;
>  
>  	struct vfs_aio_state vfs_aio_state;
> @@ -1045,7 +1046,9 @@ static void vfs_fsync_do(void *private_data)
>  		state->ret = fsync(state->fd);
>  	} while ((state->ret == -1) && (errno == EINTR));
>  
> -	state->err = errno;
> +	if (state->ret == -1) {
> +		state->vfs_aio_state.error = errno;
> +	}
>  
>  	PROFILE_TIMESTAMP(&end_time);
>  
> -- 
> 1.8.3.1
>