From 758e5e8448d2694a49f31dee99b793b2f21b66c4 Mon Sep 17 00:00:00 2001
From: Atin Mukherjee <amukherj@redhat.com>
Date: Thu, 8 Sep 2016 11:33:59 +0530
Subject: [PATCH 87/94] socket: pollerr event shouldn't trigger socket_connnect_finish
If connect fails with any other error than EINPROGRESS we cannot get
the error status using getsockopt (... SO_ERROR ... ). Hence we need
to remember the state of connect and take appropriate action in the
event_handler for the same.
As an added note, a event can come where poll_err is HUP and we have
poll_in as well (i.e some status was written to the socket), so for
such cases we need to finish the connect, process the data and then
the poll_err as is the case in the current code.
Special thanks to Kaushal M & Raghavendra G for figuring out the issue.
>Signed-off-by: Shyam <srangana@redhat.com>
>Reviewed-on: http://review.gluster.org/15440
>Smoke: Gluster Build System <jenkins@build.gluster.org>
>NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
>CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
>Reviewed-by: Raghavendra G <rgowdapp@redhat.com>
>Reviewed-on: http://review.gluster.org/15533
>Smoke: Gluster Build System <jenkins@build.gluster.org>
>NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
>CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
>Reviewed-by: Pranith Kumar Karampuri <pkarampu@redhat.com>
Change-Id: Ic45ad59ff8ab1d0a9d2cab2c924ad940b9d38528
BUG: 1377387
Signed-off-by: Atin Mukherjee <amukherj@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/85608
Reviewed-by: Milind Changire <mchangir@redhat.com>
Reviewed-by: Raghavendra Gowdappa <rgowdapp@redhat.com>
---
rpc/rpc-transport/socket/src/socket.c | 42 ++++++++++++++++++++++++++++++++-
rpc/rpc-transport/socket/src/socket.h | 3 ++
tests/bugs/changelog/bug-1211327.t | 8 ++----
tests/bugs/ec/bug-1236065.t | 4 +-
4 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/rpc/rpc-transport/socket/src/socket.c b/rpc/rpc-transport/socket/src/socket.c
index deec0cb..d9383c3 100644
--- a/rpc/rpc-transport/socket/src/socket.c
+++ b/rpc/rpc-transport/socket/src/socket.c
@@ -2346,6 +2346,7 @@ out:
return ret;
}
+static int socket_disconnect (rpc_transport_t *this);
/* reads rpc_requests during pollin */
static int
@@ -2370,7 +2371,23 @@ socket_event_handler (int fd, int idx, void *data,
}
pthread_mutex_unlock (&priv->lock);
- ret = (priv->connected == 1) ? 0 : socket_connect_finish(this);
+ if (priv->connected != 1) {
+ if (priv->connect_failed) {
+ /* connect failed with some other error than
+ EINPROGRESS or ENOENT, so nothing more to do, fail
+ reading/writing anything even if poll_in or poll_out
+ is set */
+ ret = socket_disconnect (this);
+
+ /* Force ret to be -1, as we are officially done with
+ this socket */
+ ret = -1;
+ } else {
+ ret = socket_connect_finish (this);
+ }
+ } else {
+ ret = 0;
+ }
if (!ret && poll_out) {
ret = socket_event_poll_out (this);
@@ -3046,6 +3063,16 @@ socket_connect (rpc_transport_t *this, int port)
gf_log (this->name, GF_LOG_WARNING,
"Ignore failed connection attempt on %s, (%s) ",
this->peerinfo.identifier, strerror (errno));
+
+ /* connect failed with some other error than EINPROGRESS
+ so, getsockopt (... SO_ERROR ...), will not catch any
+ errors and return them to us, we need to remember this
+ state, and take actions in socket_event_handler
+ appropriately */
+ /* TBD: What about ENOENT, we will do getsockopt there
+ as well, so how is that exempt from such a problem? */
+ priv->connect_failed = 1;
+
goto handler;
}
@@ -3058,9 +3085,22 @@ socket_connect (rpc_transport_t *this, int port)
GF_LOG_DEBUG : GF_LOG_ERROR),
"connection attempt on %s failed, (%s)",
this->peerinfo.identifier, strerror (errno));
+
+ /* connect failed with some other error than EINPROGRESS
+ so, getsockopt (... SO_ERROR ...), will not catch any
+ errors and return them to us, we need to remember this
+ state, and take actions in socket_event_handler
+ appropriately */
+ /* TBD: What about ENOENT, we will do getsockopt there
+ as well, so how is that exempt from such a problem? */
+ priv->connect_failed = 1;
+
goto handler;
}
else {
+ /* reset connect_failed so that any previous attempts
+ state is not carried forward */
+ priv->connect_failed = 0;
ret = 0;
}
diff --git a/rpc/rpc-transport/socket/src/socket.h b/rpc/rpc-transport/socket/src/socket.h
index 8395fd2..7c7005b 100644
--- a/rpc/rpc-transport/socket/src/socket.h
+++ b/rpc/rpc-transport/socket/src/socket.h
@@ -200,6 +200,9 @@ typedef struct {
int32_t idx;
/* -1 = not connected. 0 = in progress. 1 = connected */
char connected;
+ /* 1 = connect failed for reasons other than EINPROGRESS/ENOENT
+ see socket_connect for details */
+ char connect_failed;
char bio;
char connect_finish_log;
char submit_log;
diff --git a/tests/bugs/changelog/bug-1211327.t b/tests/bugs/changelog/bug-1211327.t
index 19d6e76..a849ec3 100644
--- a/tests/bugs/changelog/bug-1211327.t
+++ b/tests/bugs/changelog/bug-1211327.t
@@ -27,15 +27,13 @@ TEST $CLI volume set $V0 changelog.changelog on;
sleep 1
TEST killall_gluster;
-sleep 1
-EXPECT 0 online_brick_count;
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "0" online_brick_count
TEST glusterd;
TEST pidof glusterd;
-##Let the brick processes starts
-sleep 1;
-EXPECT 1 online_brick_count;
+##Let the brick processes starts
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "1" online_brick_count
##On brick restart only one HTIME should be found.
EXPECT 1 count_htime_files;
diff --git a/tests/bugs/ec/bug-1236065.t b/tests/bugs/ec/bug-1236065.t
index 9038cb9..e425f3e 100644
--- a/tests/bugs/ec/bug-1236065.t
+++ b/tests/bugs/ec/bug-1236065.t
@@ -48,7 +48,7 @@ TEST ec_test_make
## step 4
TEST $CLI volume start $V0 force
-EXPECT '7' online_brick_count
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "7" online_brick_count
# active heal
EXPECT_WITHIN $PROCESS_UP_TIMEOUT "[0-9][0-9]*" get_shd_process_pid
@@ -70,7 +70,7 @@ EXPECT '5' online_brick_count
## step 6
TEST $CLI volume start $V0 force
-EXPECT '7' online_brick_count
+EXPECT_WITHIN $CHILD_UP_TIMEOUT "7" online_brick_count
# self-healing
EXPECT_WITHIN $PROCESS_UP_TIMEOUT "[0-9][0-9]*" get_shd_process_pid
--
1.7.1