qpid-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From acon...@apache.org
Subject [1/2] qpid-dispatch git commit: DISPATCH-577: Server logs show unexpected POLLNVAL errors
Date Thu, 01 Dec 2016 19:15:15 GMT
Repository: qpid-dispatch
Updated Branches:
  refs/heads/master 1f0aff947 -> 69f52f283


DISPATCH-577: Server logs show unexpected POLLNVAL errors

Fixed locking mistake - must mark connector closed *before* closing socket.
Simplified poll logic.


Project: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/commit/978e8090
Tree: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/tree/978e8090
Diff: http://git-wip-us.apache.org/repos/asf/qpid-dispatch/diff/978e8090

Branch: refs/heads/master
Commit: 978e80909b44dcdd11b3b519acc7f5a36bf779c1
Parents: 1f0aff9
Author: Alan Conway <aconway@redhat.com>
Authored: Thu Dec 1 09:44:45 2016 -0500
Committer: Alan Conway <aconway@redhat.com>
Committed: Thu Dec 1 11:12:14 2016 -0500

----------------------------------------------------------------------
 include/qpid/dispatch/driver.h |  2 +-
 src/aprintf.h                  |  2 +-
 src/http-libwebsockets.c       |  4 +--
 src/posix/driver.c             | 70 ++++++++++++++++++++-----------------
 4 files changed, 41 insertions(+), 37 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/978e8090/include/qpid/dispatch/driver.h
----------------------------------------------------------------------
diff --git a/include/qpid/dispatch/driver.h b/include/qpid/dispatch/driver.h
index 28552f7..cf68776 100644
--- a/include/qpid/dispatch/driver.h
+++ b/include/qpid/dispatch/driver.h
@@ -338,7 +338,7 @@ void qdpn_connector_close(qdpn_connector_t *connector);
 
 /** Call when the socket is already closed, an the connector needs updating.
  *
- * @param[in] connector the connector whose socket will be closed
+ * @param[in] connector the connector whose socket has been closed
  */
 void qdpn_connector_after_close(qdpn_connector_t *connector);
 

http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/978e8090/src/aprintf.h
----------------------------------------------------------------------
diff --git a/src/aprintf.h b/src/aprintf.h
index f14c012..b73f7ff 100644
--- a/src/aprintf.h
+++ b/src/aprintf.h
@@ -49,7 +49,7 @@ static int vaprintf(char **begin, char *end, const char *format, va_list
ap_in)
 /**
    Appending printf.
 
-   Print to buffer at *begin with null terminatr, do not go beyond end.
+   Print to buffer at *begin with null terminator, end points after end of buffer.
    Advance *begin to point to the null terminator.
 .  Return value:
    - 0 on success: advance *begin to the null terminator.

http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/978e8090/src/http-libwebsockets.c
----------------------------------------------------------------------
diff --git a/src/http-libwebsockets.c b/src/http-libwebsockets.c
index bbc3b15..c7c0043 100644
--- a/src/http-libwebsockets.c
+++ b/src/http-libwebsockets.c
@@ -157,7 +157,7 @@ static int callback_amqpws(struct lws *wsi, enum lws_callback_reasons
reason,
 
     case LWS_CALLBACK_CLOSED: {
         qd_log(h->log, QD_LOG_DEBUG, "AMQP/WebSocket from %s closed", name);
-        qdpn_connector_after_close(c);
+        qdpn_connector_mark_closed(c);
         break;
     }
 
@@ -179,7 +179,7 @@ static int callback_http(struct lws *wsi, enum lws_callback_reasons reason,
void
         break;
     }
     case LWS_CALLBACK_CLOSED: {
-        qdpn_connector_after_close(c);
+        qdpn_connector_mark_closed(c);
     }
     default:
         break;

http://git-wip-us.apache.org/repos/asf/qpid-dispatch/blob/978e8090/src/posix/driver.c
----------------------------------------------------------------------
diff --git a/src/posix/driver.c b/src/posix/driver.c
index c4381b5..53ea809 100644
--- a/src/posix/driver.c
+++ b/src/posix/driver.c
@@ -45,13 +45,14 @@
 #include <qpid/dispatch/driver.h>
 #include <qpid/dispatch/error.h>
 #include <qpid/dispatch/threading.h>
-#include "alloc.h"
-#include "http.h"
 #include <proton/error.h>
 #include <proton/ssl.h>
 #include <proton/object.h>
 #include <qpid/dispatch/ctools.h>
-#include <qpid/dispatch/log.h>
+#include "alloc.h"
+#include "aprintf.h"
+#include "http.h"
+#include "log_private.h"
 
 /* Decls */
 
@@ -144,11 +145,17 @@ ALLOC_DEFINE(qdpn_connector_t);
 
 /* Impls */
 
-static void qdpn_log_errno(qdpn_driver_t *d, const char *msg)
+static void qdpn_log_errno(qdpn_driver_t *d, const char *fmt, ...)
 {
-    char ebuf[ERROR_MAX];
-    strerror_r(errno, ebuf, ERROR_MAX);
-    qd_log(d->log, QD_LOG_ERROR, "%s: %s", msg, ebuf);
+    char msg[QD_LOG_TEXT_MAX];
+    char *begin = msg, *end = msg+sizeof(msg);
+    va_list ap;
+    va_start(ap, fmt);
+    vaprintf(&begin, end, fmt, ap);
+    va_end(ap);
+    aprintf(&begin, end, ": ");
+    strerror_r(errno, begin, end - begin);
+    qd_log(d->log, QD_LOG_ERROR, "%s", msg);
 }
 
 
@@ -616,24 +623,25 @@ qdpn_listener_t *qdpn_connector_listener(qdpn_connector_t *ctor)
 }
 
 /* FD is already closed, update the connector state */
-void qdpn_connector_after_close(qdpn_connector_t *ctor)
+void qdpn_connector_mark_closed(qdpn_connector_t *ctor)
 {
     if (!ctor) return;
+    sys_mutex_lock(ctor->driver->lock);
     ctor->status = 0;
     if (!ctor->closed) {
-        sys_mutex_lock(ctor->driver->lock);
+        qd_log(ctor->driver->log, QD_LOG_TRACE, "closed %s", ctor->name);
         ctor->closed = true;
         ctor->driver->closed_count++;
-        sys_mutex_unlock(ctor->driver->lock);
     }
+    sys_mutex_unlock(ctor->driver->lock);
 }
 
 void qdpn_connector_close(qdpn_connector_t *ctor)
 {
     if (ctor && !ctor->closed) {
+        qdpn_connector_mark_closed(ctor);
         if (close(ctor->fd) == -1)
-            qdpn_log_errno(ctor->driver, "close");
-        qdpn_connector_after_close(ctor);
+            qdpn_log_errno(ctor->driver, "close %s", ctor->name);
     }
 }
 
@@ -734,7 +742,7 @@ static void connector_process(qdpn_connector_t *c)
             ssize_t n =  recv(c->fd, pn_transport_tail(transport), capacity, 0);
             if (n < 0) {
                 if (errno != EAGAIN) {
-                    qdpn_log_errno(c->driver, "read");
+                    qdpn_log_errno(c->driver, "recv %s", c->name);
                     pn_transport_close_tail( transport );
                 }
             } else if (n == 0) { /* HUP */
@@ -763,9 +771,8 @@ static void connector_process(qdpn_connector_t *c)
             ssize_t n = send(c->fd, pn_transport_head(transport), pending, 0);
 #endif
             if (n < 0) {
-                // XXX
                 if (errno != EAGAIN) {
-                    qdpn_log_errno(c->driver, "send");
+                    qdpn_log_errno(c->driver, "send %s", c->name);
                     pn_transport_close_head( transport );
                 }
             } else if (n) {
@@ -776,7 +783,6 @@ static void connector_process(qdpn_connector_t *c)
 
     c->status = 0;
     if (pn_transport_closed(c->transport)) {
-        qd_log(c->driver->log, QD_LOG_TRACE, "Closed %s", c->name);
         qdpn_connector_close(c);
     } else {
         if (pn_transport_capacity(transport) > 0) c->status |= PN_SEL_RD;
@@ -931,27 +937,25 @@ int qdpn_driver_wait_3(qdpn_driver_t *d)
             c->pending_read = false;
             c->pending_write = false;
             c->pending_tick = false;
-        } else {
-            int idx = c->idx;
-            c->pending_read = (idx && d->fds[idx].revents & POLLIN);
-            c->pending_write = (idx && d->fds[idx].revents & POLLOUT);
+        } else if (c->idx) {
+            short revents = d->fds[c->idx].revents;
+            c->pending_read = (revents & POLLIN);
+            c->pending_write = (revents & POLLOUT);
+            c->socket_error = (revents & POLLERR);
             c->pending_tick = (c->wakeup &&  c->wakeup <= now);
-            if (idx && d->fds[idx].revents & POLLERR) {
-                c->socket_error = true;
-            } else if (idx && (d->fds[idx].revents & POLLHUP)) {
-                qd_log(c->driver->log, QD_LOG_TRACE, "hangup on connector %s", c->name);
-                /* poll() is signalling POLLHUP. to see what happened we need
+            if (revents & ~(POLLIN|POLLOUT|POLLERR|POLLHUP)) {
+                qd_log(c->driver->log, QD_LOG_ERROR, "unexpected poll events %04x on
%s",
+                       revents, c->name);
+            }
+            if (revents & POLLHUP) {
+                qd_log(c->driver->log, QD_LOG_TRACE, "hangup on %s", c->name);
+                /* poll() is signalling POLLHUP. To see what happened we need
                  * to do an actual recv() to get the error code. But we might
                  * be in a state where we're not interested in input, in that
                  * case try to get the error code via send() */
-                if (d->fds[idx].events & POLLIN)
-                    c->pending_read = true;
-                else if (d->fds[idx].events & POLLOUT)
-                    c->pending_write = true;
-            } else if (idx && (d->fds[idx].revents & ~(POLLIN|POLLOUT|POLLERR|POLLHUP)))
{
-                qd_log(c->driver->log, QD_LOG_ERROR, "Unexpected poll events: %04x
on %s",
-                       d->fds[idx].revents, c->name);
-                c->socket_error = true;
+                short events = d->fds[c->idx].events;
+                if (events & POLLIN) c->pending_read = true;
+                else if (events & POLLOUT) c->pending_write = true;
             }
         }
         c = DEQ_NEXT(c);


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org


Mime
View raw message