[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