#213 Updates to non-blocking IO in gssproxy/client
Merged 6 years ago by cipherboy. Opened 6 years ago by cipherboy.
cipherboy/gssproxy nonblock-updates  into  master

file modified
+42 -40
@@ -1,4 +1,4 @@ 

- /* Copyright (C) 2011 the GSS-PROXY contributors, see COPYING for license */

+ /* Copyright (C) 2017 the GSS-PROXY contributors, see COPYING for license */

  

  #include "gssapi_gpm.h"

  #include <sys/types.h>
@@ -80,7 +80,6 @@ 

      struct sockaddr_un addr = {0};

      char name[PATH_MAX];

      int ret;

-     unsigned flags;

      int fd = -1;

  

      ret = get_pipe_name(name);
@@ -92,24 +91,12 @@ 

      strncpy(addr.sun_path, name, sizeof(addr.sun_path)-1);

      addr.sun_path[sizeof(addr.sun_path)-1] = '\0';

  

-     fd = socket(AF_UNIX, SOCK_STREAM, 0);

+     fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);

      if (fd == -1) {

          ret = errno;

          goto done;

      }

  

-     ret = fcntl(fd, F_GETFD, &flags);

-     if (ret != 0) {

-         ret = errno;

-         goto done;

-     }

- 

-     ret = fcntl(fd, F_SETFD, flags | O_NONBLOCK);

-     if (ret != 0) {

-         ret = errno;

-         goto done;

-     }

- 

      ret = connect(fd, (struct sockaddr *)&addr, sizeof(addr));

      if (ret == -1) {

          ret = errno;
@@ -300,26 +287,47 @@ 

          gpm_epoll_close(gpmctx);

      } else if (epoll_ret == 1 && events[0].data.fd == gpmctx->timerfd) {

          /* Got an event which is only our timer */

-         ret = read(gpmctx->timerfd, &timer_read, sizeof(uint64_t));

-         if (ret == -1 && errno != EAGAIN && errno != EWOULDBLOCK) {

-             /* In the case when reading from the timer failed, don't hide the

-              * timer error behind ETIMEDOUT such that it isn't retried */

-             ret = errno;

+         if ((events[0].events & EPOLLIN) == 0) {

+             /* We got an event which was not EPOLLIN; assume this is an error,

+              * and exit with EBADF: epoll_wait said timerfd had an event,

+              * but that event is not an EPOLIN event. */

+             ret = EBADF;

          } else {

-             /* If ret == 0, then we definitely timed out. Else, if ret == -1

-              * and errno == EAGAIN or errno == EWOULDBLOCK, we're in a weird

-              * edge case where epoll thinks the timer can be read, but it

-              * is blocking more; treat it like a TIMEOUT and retry, as

-              * nothing around us would handle EAGAIN from timer and retry

-              * it. */

-             ret = ETIMEDOUT;

+             ret = read(gpmctx->timerfd, &timer_read, sizeof(uint64_t));

+             if (ret == -1 && errno != EAGAIN && errno != EWOULDBLOCK) {

+                 /* In the case when reading from the timer failed, don't hide the

+                  * timer error behind ETIMEDOUT such that it isn't retried */

+                 ret = errno;

+             } else {

+                 /* If ret == 0, then we definitely timed out. Else, if ret == -1

+                  * and errno == EAGAIN or errno == EWOULDBLOCK, we're in a weird

+                  * edge case where epoll thinks the timer can be read, but it

+                  * is blocking more; treat it like a TIMEOUT and retry, as

+                  * nothing around us would handle EAGAIN from timer and retry

+                  * it. */

+                 ret = ETIMEDOUT;

+             }

          }

          gpm_epoll_close(gpmctx);

      } else {

          /* If ret == 2, then we ignore the timerfd; that way if the next

           * operation cannot be performed immediately, we timeout and retry.

-          * If ret == 1 and data.fd == gpmctx->fd, return 0. */

-         ret = 0;

+          * Always check the returned event of the socket fd. */

+         int fd_index = 0;

+         if (epoll_ret == 2 && events[fd_index].data.fd != gpmctx->fd) {

+             fd_index = 1;

+         }

+ 

+         if ((events[fd_index].events & event_flags) == 0) {

+             /* We cannot call EPOLLIN/EPOLLOUT at this time; assume that this

+              * is a fatal error; return with EBADFD to distinguish from

+              * EBADF in timer_fd case. */

+             ret = EBADFD;

+             gpm_epoll_close(gpmctx);

+         } else {

+             /* We definintely got a EPOLLIN/EPOLLOUT event; return success. */

+             ret = 0;

+         }

      }

  

      epoll_ret = epoll_ctl(gpmctx->epollfd, EPOLL_CTL_DEL, gpmctx->fd, NULL);
@@ -411,10 +419,7 @@ 

      ret = 0;

  

  done:

-     if (ret) {

-         /* on errors we can only close the fd and return */

-         gpm_close_socket(gpmctx);

-     }

+     /* we only need to return as gpm_retry_socket closes the socket */

      return ret;

  }

  
@@ -484,9 +489,10 @@ 

  

  done:

      if (ret) {

-         /* on errors we can only close the fd and return */

-         gpm_close_socket(gpmctx);

-         gpm_epoll_close(gpmctx);

+         /* on errors, free the buffer to prevent calling

+          * xdr_destroy(&xdr_reply_ctx); */

+         free(*buffer);

+         *buffer = NULL;

      }

      return ret;

  }
@@ -561,10 +567,6 @@ 

              /* Close and reopen socket before trying again */

              ret = gpm_retry_socket(gpmctx);

  

-             /* Free buffer and set it to NULL to prevent free(xdr_reply_ctx) */

-             free(*recv_buffer);

-             *recv_buffer = NULL;

- 

              if (ret != 0)

                  return ret;

              ret = ETIMEDOUT;

First commit simplifies setting NONBLOCK on the FD; per man 2 socket, these should be equivalent.

Second commit exposes epoll errors and validates that the proper EPOLLIN/EPOLLOUT response was received.

Third commit simplifies some error case logic (freeing buffer, closing sockets), ensuring that epollfd will be closed before the socket fd.

Fold this else branch into the previous else if for clarity.

Comment inline. Please also update copyright on that file; we should have done that when we added the code.

4 new commits added

  • Bump copyright after modifications to gpm_common.c
  • Fix error handling in gpm_send_buffer/gpm_recv_buffer.
  • Fix handling of non-EPOLLIN/EPOLLOUT events
  • Simplify setting NONBLOCK on socket.
6 years ago

Merged else block; bumped copyright.

Commit f2530fc fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago

Commit b8f5b2f fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago

Commit ec808ee fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago

Commit 8f51999 fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago

Commit f2530fc fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago

Commit 9065aba fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago

Commit b8f5b2f fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago

Commit 5ed25ce fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago

Commit ec808ee fixes this pull-request

Pull-Request has been merged by alexander.m.scheel@gmail.com

6 years ago