Patchwork supplicant: Make unix socket non-blocking.

login
register
mail settings
Submitter Ben Greear
Date Jan. 5, 2012, 6:52 p.m.
Message ID <1325789566-2428-1-git-send-email-greearb@candelatech.com>
Download mbox | patch
Permalink /patch/134534/
State Superseded
Headers show

Comments

Ben Greear - Jan. 5, 2012, 6:52 p.m.
From: Ben Greear <greearb@candelatech.com>

This keeps wpa_cli from hanging forever if the other end of the
socket dies.

Signed-hostap: Ben Greear <greearb@candelatech.com>
---
:100644 100644 3b25f77... 8ed9dee... M	src/common/wpa_ctrl.c
:100644 100644 306a222... 113d361... M	wpa_supplicant/ctrl_iface_unix.c
 src/common/wpa_ctrl.c            |   23 +++++++++++++++++++++++
 wpa_supplicant/ctrl_iface_unix.c |   15 ++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletions(-)
Ben Greear - Jan. 30, 2012, 7:03 p.m.
On 01/05/2012 10:52 AM, greearb@candelatech.com wrote:
> From: Ben Greear<greearb@candelatech.com>
>
> This keeps wpa_cli from hanging forever if the other end of the
> socket dies.

Hello Jouni:

I've been posting this patch for a while, and it has
neither gotten picked up nor commented upon.

If you get a chance, please let me know if this
patch needs more work to be accepted.  If you just
don't want the patch..let me know and
I'll quit worrying about it.

Thanks,
Ben

>
> Signed-hostap: Ben Greear<greearb@candelatech.com>
> ---
> :100644 100644 3b25f77... 8ed9dee... M	src/common/wpa_ctrl.c
> :100644 100644 306a222... 113d361... M	wpa_supplicant/ctrl_iface_unix.c
>   src/common/wpa_ctrl.c            |   23 +++++++++++++++++++++++
>   wpa_supplicant/ctrl_iface_unix.c |   15 ++++++++++++++-
>   2 files changed, 37 insertions(+), 1 deletions(-)
>
> diff --git a/src/common/wpa_ctrl.c b/src/common/wpa_ctrl.c
> index 3b25f77..8ed9dee 100644
> --- a/src/common/wpa_ctrl.c
> +++ b/src/common/wpa_ctrl.c
> @@ -295,6 +295,7 @@ int wpa_ctrl_request(struct wpa_ctrl *ctrl, const char *cmd, size_t cmd_len,
>   		     void (*msg_cb)(char *msg, size_t len))
>   {
>   	struct timeval tv;
> +	struct os_time started_at;
>   	int res;
>   	fd_set rfds;
>   	const char *_cmd;
> @@ -321,7 +322,29 @@ int wpa_ctrl_request(struct wpa_ctrl *ctrl, const char *cmd, size_t cmd_len,
>   		_cmd_len = cmd_len;
>   	}
>
> +	errno = 0;
> +	started_at.sec = 0;
> +	started_at.usec = 0;
> +retry_send:
>   	if (send(ctrl->s, _cmd, _cmd_len, 0)<  0) {
> +		if ((errno == EAGAIN) || (errno = EBUSY)
> +		    || (errno == EWOULDBLOCK)) {
> +			/* Must be non-blocking socket...try for a bit longer
> +			 * before giving up.
> +			 */
> +			if (started_at.sec == 0)
> +				os_get_time(&started_at);
> +			else {
> +				struct os_time n;
> +				os_get_time(&n);
> +				/* Try for a few seconds. */
> +				if (n.sec>  (started_at.sec + 5))
> +					goto send_err;
> +			}
> +			os_sleep(1, 0);
> +			goto retry_send;
> +		}
> +	send_err:
>   		os_free(cmd_buf);
>   		return -1;
>   	}
> diff --git a/wpa_supplicant/ctrl_iface_unix.c b/wpa_supplicant/ctrl_iface_unix.c
> index 306a222..113d361 100644
> --- a/wpa_supplicant/ctrl_iface_unix.c
> +++ b/wpa_supplicant/ctrl_iface_unix.c
> @@ -20,7 +20,8 @@
>   #ifdef ANDROID
>   #include<cutils/sockets.h>
>   #endif /* ANDROID */
> -
> +#include<unistd.h>
> +#include<fcntl.h>
>   #include "utils/common.h"
>   #include "utils/eloop.h"
>   #include "utils/list.h"
> @@ -265,6 +266,7 @@ wpa_supplicant_ctrl_iface_init(struct wpa_supplicant *wpa_s)
>   	char *buf, *dir = NULL, *gid_str = NULL;
>   	struct group *grp;
>   	char *endp;
> +	int  flags;
>
>   	priv = os_zalloc(sizeof(*priv));
>   	if (priv == NULL)
> @@ -411,6 +413,17 @@ wpa_supplicant_ctrl_iface_init(struct wpa_supplicant *wpa_s)
>   #ifdef ANDROID
>   havesock:
>   #endif /* ANDROID */
> +
> +	/* Make socket non-blocking so that we don't hang forever if
> +	 * target dies unexpectedly.
> +	 */
> +	flags = fcntl(priv->sock, F_GETFL);
> +	flags |= (O_NONBLOCK);
> +	if (fcntl(priv->sock, F_SETFL, flags)<  0) {
> +		perror("fcntl(ctrl, O_NONBLOCK)");
> +		/* Not fatal, continue on.*/
> +	}
> +
>   	eloop_register_read_sock(priv->sock, wpa_supplicant_ctrl_iface_receive,
>   				 wpa_s, priv);
>   	wpa_msg_register_cb(wpa_supplicant_ctrl_iface_msg_cb);
Jouni Malinen - Jan. 30, 2012, 8:02 p.m.
On Mon, Jan 30, 2012 at 11:03:17AM -0800, Ben Greear wrote:
> I've been posting this patch for a while, and it has
> neither gotten picked up nor commented upon.

Yeah.. Looks like others don't care too much and I just have not had
chance to convince myself that this is the way to go yet..

> If you get a chance, please let me know if this
> patch needs more work to be accepted.  If you just
> don't want the patch..let me know and
> I'll quit worrying about it.

It is on my queue - I just haven't decided yet whether I like it or not
;-).
Jouni Malinen - Feb. 5, 2012, 5:18 p.m.
On Thu, Jan 05, 2012 at 10:52:46AM -0800, greearb@candelatech.com wrote:
> This keeps wpa_cli from hanging forever if the other end of the
> socket dies.

Hmm.. The change here is for wpa_cli:

> diff --git a/src/common/wpa_ctrl.c b/src/common/wpa_ctrl.c
> @@ -321,7 +322,29 @@ int wpa_ctrl_request(struct wpa_ctrl *ctrl, const char *cmd, size_t cmd_len,
> +retry_send:
>  	if (send(ctrl->s, _cmd, _cmd_len, 0) < 0) {
> +		if ((errno == EAGAIN) || (errno = EBUSY)
> +		    || (errno == EWOULDBLOCK)) {
> +			/* Must be non-blocking socket...try for a bit longer
> +			 * before giving up.
> +			 */

...

while the part that makes the socket non-blocking is for wpa_supplicant:

> diff --git a/wpa_supplicant/ctrl_iface_unix.c b/wpa_supplicant/ctrl_iface_unix.c
> @@ -411,6 +413,17 @@ wpa_supplicant_ctrl_iface_init(struct wpa_supplicant *wpa_s)
> +	/* Make socket non-blocking so that we don't hang forever if
> +	 * target dies unexpectedly.
> +	 */
> +	flags = fcntl(priv->sock, F_GETFL);
> +	flags |= (O_NONBLOCK);
> +	if (fcntl(priv->sock, F_SETFL, flags) < 0) {

Am I missing something here or how is this supposed to work? Shouldn't
these changes be for the same socket, i.e., the one used by wpa_cli, or
were you thinking of making both wpa_cli and wpa_supplicant use
non-blocking sockets (in which case this fnctl call would need to be in
wpa_ctrl.c, too, I'd assume)?
Ben Greear - April 6, 2012, 8:18 p.m.
On 02/05/2012 09:18 AM, Jouni Malinen wrote:
> On Thu, Jan 05, 2012 at 10:52:46AM -0800, greearb@candelatech.com wrote:
>> This keeps wpa_cli from hanging forever if the other end of the
>> socket dies.
>
> Hmm.. The change here is for wpa_cli:
>
>> diff --git a/src/common/wpa_ctrl.c b/src/common/wpa_ctrl.c
>> @@ -321,7 +322,29 @@ int wpa_ctrl_request(struct wpa_ctrl *ctrl, const char *cmd, size_t cmd_len,
>> +retry_send:
>>   	if (send(ctrl->s, _cmd, _cmd_len, 0)<  0) {
>> +		if ((errno == EAGAIN) || (errno = EBUSY)
>> +		    || (errno == EWOULDBLOCK)) {
>> +			/* Must be non-blocking socket...try for a bit longer
>> +			 * before giving up.
>> +			 */
>
> ...
>
> while the part that makes the socket non-blocking is for wpa_supplicant:
>
>> diff --git a/wpa_supplicant/ctrl_iface_unix.c b/wpa_supplicant/ctrl_iface_unix.c
>> @@ -411,6 +413,17 @@ wpa_supplicant_ctrl_iface_init(struct wpa_supplicant *wpa_s)
>> +	/* Make socket non-blocking so that we don't hang forever if
>> +	 * target dies unexpectedly.
>> +	 */
>> +	flags = fcntl(priv->sock, F_GETFL);
>> +	flags |= (O_NONBLOCK);
>> +	if (fcntl(priv->sock, F_SETFL, flags)<  0) {
>
> Am I missing something here or how is this supposed to work? Shouldn't
> these changes be for the same socket, i.e., the one used by wpa_cli, or
> were you thinking of making both wpa_cli and wpa_supplicant use
> non-blocking sockets (in which case this fnctl call would need to be in
> wpa_ctrl.c, too, I'd assume)?


I think you are right.  I should be adding the fcntl to wpa_ctrl.c
as well.

I've updated the patch in my tree and will re-post after
it gets a bit of testing time.

Thanks,
Ben

Patch

diff --git a/src/common/wpa_ctrl.c b/src/common/wpa_ctrl.c
index 3b25f77..8ed9dee 100644
--- a/src/common/wpa_ctrl.c
+++ b/src/common/wpa_ctrl.c
@@ -295,6 +295,7 @@  int wpa_ctrl_request(struct wpa_ctrl *ctrl, const char *cmd, size_t cmd_len,
 		     void (*msg_cb)(char *msg, size_t len))
 {
 	struct timeval tv;
+	struct os_time started_at;
 	int res;
 	fd_set rfds;
 	const char *_cmd;
@@ -321,7 +322,29 @@  int wpa_ctrl_request(struct wpa_ctrl *ctrl, const char *cmd, size_t cmd_len,
 		_cmd_len = cmd_len;
 	}
 
+	errno = 0;
+	started_at.sec = 0;
+	started_at.usec = 0;
+retry_send:
 	if (send(ctrl->s, _cmd, _cmd_len, 0) < 0) {
+		if ((errno == EAGAIN) || (errno = EBUSY)
+		    || (errno == EWOULDBLOCK)) {
+			/* Must be non-blocking socket...try for a bit longer
+			 * before giving up.
+			 */
+			if (started_at.sec == 0)
+				os_get_time(&started_at);
+			else {
+				struct os_time n;
+				os_get_time(&n);
+				/* Try for a few seconds. */
+				if (n.sec > (started_at.sec + 5))
+					goto send_err;
+			}
+			os_sleep(1, 0);
+			goto retry_send;
+		}
+	send_err:
 		os_free(cmd_buf);
 		return -1;
 	}
diff --git a/wpa_supplicant/ctrl_iface_unix.c b/wpa_supplicant/ctrl_iface_unix.c
index 306a222..113d361 100644
--- a/wpa_supplicant/ctrl_iface_unix.c
+++ b/wpa_supplicant/ctrl_iface_unix.c
@@ -20,7 +20,8 @@ 
 #ifdef ANDROID
 #include <cutils/sockets.h>
 #endif /* ANDROID */
-
+#include <unistd.h>
+#include <fcntl.h>
 #include "utils/common.h"
 #include "utils/eloop.h"
 #include "utils/list.h"
@@ -265,6 +266,7 @@  wpa_supplicant_ctrl_iface_init(struct wpa_supplicant *wpa_s)
 	char *buf, *dir = NULL, *gid_str = NULL;
 	struct group *grp;
 	char *endp;
+	int  flags;
 
 	priv = os_zalloc(sizeof(*priv));
 	if (priv == NULL)
@@ -411,6 +413,17 @@  wpa_supplicant_ctrl_iface_init(struct wpa_supplicant *wpa_s)
 #ifdef ANDROID
 havesock:
 #endif /* ANDROID */
+
+	/* Make socket non-blocking so that we don't hang forever if
+	 * target dies unexpectedly.
+	 */
+	flags = fcntl(priv->sock, F_GETFL);
+	flags |= (O_NONBLOCK);
+	if (fcntl(priv->sock, F_SETFL, flags) < 0) {
+		perror("fcntl(ctrl, O_NONBLOCK)");
+		/* Not fatal, continue on.*/
+	}
+
 	eloop_register_read_sock(priv->sock, wpa_supplicant_ctrl_iface_receive,
 				 wpa_s, priv);
 	wpa_msg_register_cb(wpa_supplicant_ctrl_iface_msg_cb);