| Message ID | 1325789566-2428-1-git-send-email-greearb@candelatech.com |
|---|---|
| State | Superseded |
| Headers | show |
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);
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 ;-).
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)?
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
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);
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(-)