[PATCH] fix picky build (Re: vfs_glusterfs: Fix AIO crash on smb.conf reload.)

Michael Adam obnox at samba.org
Tue Dec 15 14:10:00 UTC 2015


On 2015-12-15 at 07:37 -0500, Ira Cooper wrote:
> NAK,
> 
> I'll get the correct patch out today after testing it today on USA time.
> 
> That destructor must be attached.  (I've got the patch, but I need to test
> it.)

Ok, I kind of guessed that, but since there was no reference
whatsoever, I thought it might just have been a remnant of
an earlier version of the patch...

Michael

> On Tue, Dec 15, 2015 at 2:39 AM, Michael Adam <obnox at samba.org> wrote:
> 
> > Apparently, the discussion has been continued offline
> > and an updated patch(set) has been pushed meanwhile...
> >
> > It breaks the --picky-devloper build.
> > Attach find a patch to fix this.
> >
> > Cheers - Michael
> >
> >
> > On 2015-12-10 at 16:57 -0800, Jeremy Allison wrote:
> > > On Thu, Dec 10, 2015 at 04:37:36PM -0500, Ira Cooper wrote:
> > > > This is really a bug related to how vfs_glusterfs handles AIO and
> > > > cancellation, so new logic was needed on those paths.
> > > >
> > > > The patch enclosed passes smoke tests on my laptop at this point, and
> > it
> > > > is passing valgrind.
> > >
> > > Just a few comments (inline) below. Sorry if I'm being too picky about
> > > the whitespace changes but any extra whitespace mods (IMHO)
> > > hide the actual changes. Feel free to tell me to bugger off :-).
> > >
> > > Cheers,
> > >
> > >       Jeremy.
> > >
> > > >      */
> > > >
> > > > -   sts = sys_read(read_fd, &req, sizeof(struct tevent_req *));
> > > > +   sts = sys_read(read_fd, &state, sizeof(struct glusterfs_aio_state
> > *));
> > > > +
> > > > +   if (state->cancelled) {
> > > > +           goto done;
> > > > +   }
> > > > +
> > > > +   req = state->req;
> > > > +
> > > >     if (sts < 0) {
> > > >             DEBUG(0,("\nRead from pipe failed (%s)", strerror(errno)));
> > > >     }
> > >
> > > We need to have the if (sts < 0) check before the use
> > > of state->cancelled and state->req above.
> > >
> > > > +   /* if we've cancelled the op, there is no req, so just clean up. */
> > > > +
> > > > +   if (state->cancelled == true) {
> > > > +           TALLOC_FREE(state);
> > > > +           goto done;
> > > > +   }
> > > > +
> > > >     if (req) {
> > > >             tevent_req_done(req);
> > > >     }
> > > > +
> > > > + done:
> > > >     return;
> > > >  }
> > >
> > > Do we really need the goto done and done: label ? I'd prefer
> > > to use immediate return; statements here (early return is
> > > always nicer to me).
> > >
> > > > @@ -581,6 +614,7 @@ static bool init_gluster_aio(struct
> > vfs_handle_struct *handle)
> > > >     }
> > > >
> > > >     ret = pipe(fds);
> > > > +
> > >
> > > Unneeded whitespace change :-).
> > >
> > > >     if (ret == -1) {
> > > >             goto fail;
> > > >     }
> > > > @@ -610,28 +644,72 @@ fail:
> > > >     return false;
> > > >  }
> > > >
> > > > -static struct tevent_req *vfs_gluster_pread_send(struct
> > vfs_handle_struct
> > > > -                                            *handle, TALLOC_CTX
> > *mem_ctx,
> > > > -                                            struct tevent_context *ev,
> > > > -                                            files_struct *fsp, void
> > *data,
> > > > -                                            size_t n, off_t offset)
> > > > +typedef int (*gluster_aio_p_fn)(glfs_fd_t *,
> > > > +                           void *,
> > > > +                           size_t,
> > > > +                           off_t,
> > > > +                           int,
> > > > +                           glfs_io_cbk,
> > > > +                           void *);
> > > > +
> > > > +
> > > > +static struct glusterfs_aio_state *aio_state_create(TALLOC_CTX
> > *mem_ctx)
> > > >  {
> > > >     struct tevent_req *req = NULL;
> > > >     struct glusterfs_aio_state *state = NULL;
> > > > -   int ret = 0;
> > > > +   struct glusterfs_aio_wrapper *wrapper = NULL;
> > > > +
> > > > +   req = tevent_req_create(mem_ctx, &wrapper, struct
> > glusterfs_aio_wrapper);
> > > >
> > > > -   req = tevent_req_create(mem_ctx, &state, struct
> > glusterfs_aio_state);
> > > >     if (req == NULL) {
> > > >             return NULL;
> > > >     }
> > > >
> > > > +   state = talloc(NULL, struct glusterfs_aio_state);
> > > > +
> > > > +   if (state == NULL) {
> > > > +           TALLOC_FREE(req);
> > > > +           return NULL;
> > > > +   }
> > > > +
> > > > +   state->cancelled = false;
> > > > +   state->ret = false;
> > > > +   state->err = false;
> > > > +   state->req = req;
> > > > +
> > > > +   wrapper->state = state;
> > > > +
> > > > +   return state;
> > > > +}
> > > > +
> > > > +
> > > > +static struct tevent_req *vfs_gluster_pwrite_send(struct
> > vfs_handle_struct
> > > > +                                             *handle, TALLOC_CTX
> > *mem_ctx,
> > > > +                                             struct tevent_context
> > *ev,
> > > > +                                             files_struct *fsp,
> > > > +                                             const void *data, size_t
> > n,
> > > > +                                             off_t offset)
> > > > +{
> > > > +   struct glusterfs_aio_state *state = NULL;
> > > > +   struct tevent_req *req = NULL;
> > >
> > > Do we need the extra 'req' variable here ? As we only
> > > refer to it a couple of times can't we just use state->req
> > > in both cases instead ?
> > >
> > > > +   int ret = 0;
> > > > +
> > > > +   state = aio_state_create(mem_ctx);
> > > > +
> > > > +   if (state == NULL) {
> > > > +           return NULL;
> > > > +   }
> > > > +
> > > > +   req = state->req;
> > > > +
> > > >     if (!init_gluster_aio(handle)) {
> > > >             tevent_req_error(req, EIO);
> > > >             return tevent_req_post(req, ev);
> > > >     }
> > > > -   ret = glfs_pread_async(*(glfs_fd_t
> > **)VFS_FETCH_FSP_EXTENSION(handle,
> > > > -                           fsp), data, n, offset, 0,
> > aio_glusterfs_done,
> > > > -                           req);
> > > > +
> > > > +   ret = glfs_pwrite_async(*(glfs_fd_t
> > **)VFS_FETCH_FSP_EXTENSION(handle, fsp),
> > > > +                          data, n, offset, 0, aio_glusterfs_done,
> > state);
> > > > +
> > > >     if (ret < 0) {
> > > >             tevent_req_error(req, -ret);
> > > >             return tevent_req_post(req, ev);
> > > > @@ -640,53 +718,68 @@ static struct tevent_req
> > *vfs_gluster_pread_send(struct vfs_handle_struct
> > > >     return req;
> > > >  }
> > > >
> > > > -static ssize_t vfs_gluster_write(struct vfs_handle_struct *handle,
> > > > -                            files_struct *fsp, const void *data,
> > size_t n)
> > > > -{
> > > > -   return glfs_write(*(glfs_fd_t **)VFS_FETCH_FSP_EXTENSION(handle,
> > fsp), data, n, 0);
> > > > -}
> > > > -
> > > > -static ssize_t vfs_gluster_pwrite(struct vfs_handle_struct *handle,
> > > > -                             files_struct *fsp, const void *data,
> > > > -                             size_t n, off_t offset)
> > > > -{
> > > > -   return glfs_pwrite(*(glfs_fd_t **)VFS_FETCH_FSP_EXTENSION(handle,
> > fsp), data, n, offset, 0);
> > > > -}
> > > > -
> > > > -static struct tevent_req *vfs_gluster_pwrite_send(struct
> > vfs_handle_struct
> > > > +static struct tevent_req *vfs_gluster_pread_send(struct
> > vfs_handle_struct
> > > >                                               *handle, TALLOC_CTX
> > *mem_ctx,
> > > >                                               struct tevent_context
> > *ev,
> > > >                                               files_struct *fsp,
> > > > -                                             const void *data, size_t
> > n,
> > > > +                                             void *data, size_t n,
> > > >                                               off_t offset)
> > > >  {
> > > > -   struct tevent_req *req = NULL;
> > > >     struct glusterfs_aio_state *state = NULL;
> > > > +   struct tevent_req *req = NULL;
> > >
> > > Same deal as above with the extra 'req' variable.
> > >
> > > >     int ret = 0;
> > > >
> > > > -   req = tevent_req_create(mem_ctx, &state, struct
> > glusterfs_aio_state);
> > > > -   if (req == NULL) {
> > > > +   state = aio_state_create(mem_ctx);
> > > > +
> > > > +   if (state == NULL) {
> > > >             return NULL;
> > > >     }
> > > > +
> > > > +   req = state->req;
> > > > +
> > > >     if (!init_gluster_aio(handle)) {
> > > >             tevent_req_error(req, EIO);
> > > >             return tevent_req_post(req, ev);
> > > >     }
> > > > -   ret = glfs_pwrite_async(*(glfs_fd_t
> > **)VFS_FETCH_FSP_EXTENSION(handle,
> > > > -                           fsp), data, n, offset, 0,
> > aio_glusterfs_done,
> > > > -                           req);
> > > > +
> > > > +   ret = glfs_pread_async(*(glfs_fd_t
> > **)VFS_FETCH_FSP_EXTENSION(handle, fsp),
> > > > +                          data, n, offset, 0, aio_glusterfs_done,
> > state);
> > > > +
> > > >     if (ret < 0) {
> > > > -           tevent_req_error(req, -ret);
> > > >             return tevent_req_post(req, ev);
> > > > +           tevent_req_error(req, -ret);
> > >
> > > I think the two lines above are transposed.
> > > Should be:
> > >
> > >                 tevent_req_error(req, -ret);
> > >                 return tevent_req_post(req, ev);
> > >
> > > as in the vfs_gluster_pread_send() code.
> > >
> > > >     }
> > > > +
> > > >     return req;
> > > >  }
> > > >
> > > > +static ssize_t vfs_gluster_write(struct vfs_handle_struct *handle,
> > > > +                            files_struct *fsp, const void *data,
> > size_t n)
> > > > +{
> > > > +   return glfs_write(*(glfs_fd_t **)VFS_FETCH_FSP_EXTENSION(handle,
> > fsp), data, n, 0);
> > > > +}
> > > > +
> > > > +static ssize_t vfs_gluster_pwrite(struct vfs_handle_struct *handle,
> > > > +                             files_struct *fsp, const void *data,
> > > > +                             size_t n, off_t offset)
> > > > +{
> > > > +   return glfs_pwrite(*(glfs_fd_t **)VFS_FETCH_FSP_EXTENSION(handle,
> > fsp), data, n, offset, 0);
> > > > +}
> > > > +
> > > >  static ssize_t vfs_gluster_recv(struct tevent_req *req, int *err)
> > > >  {
> > > >     struct glusterfs_aio_state *state = NULL;
> > > > +   struct glusterfs_aio_wrapper *wrapper = NULL;
> > > > +   int ret = 0;
> > > > +
> > > > +   wrapper = tevent_req_data(req, struct glusterfs_aio_wrapper);
> > > > +
> > > > +   if (wrapper == NULL) {
> > > > +           return -1;
> > > > +   }
> > > > +
> > > > +   state = wrapper->state;
> > > >
> > > > -   state = tevent_req_data(req, struct glusterfs_aio_state);
> > > >     if (state == NULL) {
> > > >             return -1;
> > > >     }
> > > > @@ -697,7 +790,14 @@ static ssize_t vfs_gluster_recv(struct tevent_req
> > *req, int *err)
> > > >     if (state->ret == -1) {
> > > >             *err = state->err;
> > > >     }
> > > > -   return state->ret;
> > > > +
> > > > +   ret = state->ret;
> > > > +
> > > > +   /* Clean up the state, it is in a NULL context. */
> > > > +
> > > > +   TALLOC_FREE(state);
> > > > +
> > > > +   return ret;
> > > >  }
> > > >
> > > >  static off_t vfs_gluster_lseek(struct vfs_handle_struct *handle,
> > > > @@ -746,20 +846,26 @@ static struct tevent_req
> > *vfs_gluster_fsync_send(struct vfs_handle_struct
> > > >     struct glusterfs_aio_state *state = NULL;
> > > >     int ret = 0;
> > > >
> > > > -   req = tevent_req_create(mem_ctx, &state, struct
> > glusterfs_aio_state);
> > > > -   if (req == NULL) {
> > > > +   state = aio_state_create(mem_ctx);
> > > > +
> > > > +   if (state == NULL) {
> > > >             return NULL;
> > > >     }
> > > > +
> > > > +   req = state->req;
> > > > +
> > > >     if (!init_gluster_aio(handle)) {
> > > >             tevent_req_error(req, EIO);
> > > >             return tevent_req_post(req, ev);
> > > >     }
> > > >     ret = glfs_fsync_async(*(glfs_fd_t
> > **)VFS_FETCH_FSP_EXTENSION(handle,
> > > >                             fsp), aio_glusterfs_done, req);
> > > > +
> > > Do we really need whitespace changes here ?
> > > >     if (ret < 0) {
> > > >             tevent_req_error(req, -ret);
> > > >             return tevent_req_post(req, ev);
> > > >     }
> > > > +
> > > and here ?
> > > >     return req;
> > > >  }
> > > >
> > > > --
> > > > 2.5.0
> > > >
> > >
> > >
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151215/1e2c54e8/signature.sig>


More information about the samba-technical mailing list