Re: connect_sync
On Fri, 19 Aug 2016 19:14:54 -0400, "Ted Unangst" wrote:
> hmm. so I was trying to avoid the need for two different functions. I think
> there's a mental overhead to "do this, then maybe that". this loop reads
> very strangely to me. it's hard to mentally trace the code. it's not really a
> loop, just goto spelled with break and continue.
I suppose it is better to just use connect() and connect_wait() in
a for() loop. Whichever way you choose we should put as an example
in connect(2).
- todd
Index: usr.bin/ftp/extern.h
===
RCS file: /cvs/src/usr.bin/ftp/extern.h,v
retrieving revision 1.43
diff -u -p -u -r1.43 extern.h
--- usr.bin/ftp/extern.h18 Aug 2016 16:23:06 - 1.43
+++ usr.bin/ftp/extern.h19 Aug 2016 21:54:32 -
@@ -62,7 +62,6 @@
*/
#include
-#include
void abort_remote(FILE *);
void abortpt(int);
@@ -76,7 +75,7 @@ void cmdabort(int);
void cmdscanner(int);
intcommand(const char *, ...);
intconfirm(const char *, const char *);
-intconnect_sync(int, const struct sockaddr *, socklen_t);
+intconnect_wait(int);
FILE *dataconn(const char *);
intforegroundproc(void);
intfileindir(const char *, const char *);
Index: usr.bin/ftp/fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.148
diff -u -p -u -r1.148 fetch.c
--- usr.bin/ftp/fetch.c 18 Aug 2016 16:23:06 - 1.148
+++ usr.bin/ftp/fetch.c 19 Aug 2016 23:28:11 -
@@ -557,8 +557,10 @@ noslash:
}
#endif /* !SMALL */
-again:
- if (connect_sync(s, res->ai_addr, res->ai_addrlen) < 0) {
+ for (error = connect(s, res->ai_addr, res->ai_addrlen);
+ error != 0 && errno == EINTR; error = connect_wait(s))
+ continue;
+ if (error != 0) {
save_errno = errno;
close(s);
errno = save_errno;
Index: usr.bin/ftp/ftp.c
===
RCS file: /cvs/src/usr.bin/ftp/ftp.c,v
retrieving revision 1.98
diff -u -p -u -r1.98 ftp.c
--- usr.bin/ftp/ftp.c 18 Aug 2016 16:23:06 - 1.98
+++ usr.bin/ftp/ftp.c 19 Aug 2016 23:28:22 -
@@ -219,8 +219,10 @@ hookup(char *host, char *port)
}
}
#endif /* !SMALL */
- error = connect_sync(s, res->ai_addr, res->ai_addrlen);
- if (error) {
+ for (error = connect(s, res->ai_addr, res->ai_addrlen);
+ error != 0 && errno == EINTR; error = connect_wait(s))
+ continue;
+ if (error != 0) {
/* this "if" clause is to prevent print warning twice */
if (verbose && res->ai_next) {
if (getnameinfo(res->ai_addr, res->ai_addrlen,
@@ -1514,8 +1516,11 @@ reinit:
} else
goto bad;
- if (connect_sync(data, (struct sockaddr *)_addr,
- data_addr.su_len) < 0) {
+ for (error = connect(data, (struct sockaddr *)_addr,
+ data_addr.su_len); error != 0 && errno == EINTR;
+ error = connect_wait(data))
+ continue;
+ if (error != 0) {
if (activefallback) {
(void)close(data);
data = -1;
Index: usr.bin/ftp/util.c
===
RCS file: /cvs/src/usr.bin/ftp/util.c,v
retrieving revision 1.80
diff -u -p -u -r1.80 util.c
--- usr.bin/ftp/util.c 18 Aug 2016 16:23:06 - 1.80
+++ usr.bin/ftp/util.c 19 Aug 2016 22:03:10 -
@@ -67,6 +67,7 @@
* FTP User Program -- Misc support routines
*/
#include
+#include
#include
#include
@@ -1070,35 +1071,25 @@ controlediting(void)
#endif /* !SMALL */
/*
- * Wrapper for connect(2) that restarts the syscall when
- * interrupted and operates synchronously.
+ * Wait for an asynchronous connect(2) attempt to finish.
*/
int
-connect_sync(int s, const struct sockaddr *name, socklen_t namelen)
+connect_wait(int s)
{
struct pollfd pfd[1];
int error = 0;
socklen_t len = sizeof(error);
- if (connect(s, name, namelen) < 0) {
- if (errno != EINTR)
- return -1;
- }
-
- /* An interrupted connect(2) continues asyncronously. */
pfd[0].fd = s;
pfd[0].events = POLLOUT;
- for (;;) {
- if (poll(pfd, 1, -1) == -1) {
- if (errno != EINTR)
- return -1;
- continue;
- }
- if (getsockopt(s, SOL_SOCKET, SO_ERROR, , ) < 0)
-
Re: connect_sync
Todd C. Miller wrote:
> Here's a rewrite of my connect_sync() changes to use connect_wait()
> instead. Unlike the version in the connect(2) manual, this one
> returns EINTR when interrupted by a signal which is probably better.
> Index: usr.bin/ftp/fetch.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
> retrieving revision 1.148
> diff -u -p -u -r1.148 fetch.c
> --- usr.bin/ftp/fetch.c 18 Aug 2016 16:23:06 - 1.148
> +++ usr.bin/ftp/fetch.c 19 Aug 2016 22:00:26 -
> @@ -557,8 +557,12 @@ noslash:
> }
> #endif /* !SMALL */
>
> -again:
> - if (connect_sync(s, res->ai_addr, res->ai_addrlen) < 0) {
> + error = connect(s, res->ai_addr, res->ai_addrlen);
> + while (error != 0) {
> + if (errno == EINTR) {
> + error = connect_wait(s);
> + continue;
> + }
> save_errno = errno;
> close(s);
> errno = save_errno;
continue;
hmm. so I was trying to avoid the need for two different functions. I think
there's a mental overhead to "do this, then maybe that". this loop reads
very strangely to me. it's hard to mentally trace the code. it's not really a
loop, just goto spelled with break and continue.
there's a bug, too, since the existing continue wasn't replaced. now you need
some break/continue the other loop dance.
Re: connect_sync
Here's a rewrite of my connect_sync() changes to use connect_wait()
instead. Unlike the version in the connect(2) manual, this one
returns EINTR when interrupted by a signal which is probably better.
- todd
Index: usr.bin/ftp/extern.h
===
RCS file: /cvs/src/usr.bin/ftp/extern.h,v
retrieving revision 1.43
diff -u -p -u -r1.43 extern.h
--- usr.bin/ftp/extern.h18 Aug 2016 16:23:06 - 1.43
+++ usr.bin/ftp/extern.h19 Aug 2016 21:54:32 -
@@ -62,7 +62,6 @@
*/
#include
-#include
void abort_remote(FILE *);
void abortpt(int);
@@ -76,7 +75,7 @@ void cmdabort(int);
void cmdscanner(int);
intcommand(const char *, ...);
intconfirm(const char *, const char *);
-intconnect_sync(int, const struct sockaddr *, socklen_t);
+intconnect_wait(int);
FILE *dataconn(const char *);
intforegroundproc(void);
intfileindir(const char *, const char *);
Index: usr.bin/ftp/fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.148
diff -u -p -u -r1.148 fetch.c
--- usr.bin/ftp/fetch.c 18 Aug 2016 16:23:06 - 1.148
+++ usr.bin/ftp/fetch.c 19 Aug 2016 22:00:26 -
@@ -557,8 +557,12 @@ noslash:
}
#endif /* !SMALL */
-again:
- if (connect_sync(s, res->ai_addr, res->ai_addrlen) < 0) {
+ error = connect(s, res->ai_addr, res->ai_addrlen);
+ while (error != 0) {
+ if (errno == EINTR) {
+ error = connect_wait(s);
+ continue;
+ }
save_errno = errno;
close(s);
errno = save_errno;
Index: usr.bin/ftp/ftp.c
===
RCS file: /cvs/src/usr.bin/ftp/ftp.c,v
retrieving revision 1.98
diff -u -p -u -r1.98 ftp.c
--- usr.bin/ftp/ftp.c 18 Aug 2016 16:23:06 - 1.98
+++ usr.bin/ftp/ftp.c 19 Aug 2016 22:02:35 -
@@ -219,8 +219,12 @@ hookup(char *host, char *port)
}
}
#endif /* !SMALL */
- error = connect_sync(s, res->ai_addr, res->ai_addrlen);
- if (error) {
+ error = connect(s, res->ai_addr, res->ai_addrlen);
+ while (error != 0) {
+ if (errno == EINTR) {
+ error = connect_wait(s);
+ continue;
+ }
/* this "if" clause is to prevent print warning twice */
if (verbose && res->ai_next) {
if (getnameinfo(res->ai_addr, res->ai_addrlen,
@@ -235,11 +239,12 @@ hookup(char *host, char *port)
close(s);
errno = error;
s = -1;
- continue;
+ break;
}
/* finally we got one */
- break;
+ if (error == 0)
+ break;
}
if (s < 0) {
warn("%s", cause);
@@ -1514,8 +1519,13 @@ reinit:
} else
goto bad;
- if (connect_sync(data, (struct sockaddr *)_addr,
- data_addr.su_len) < 0) {
+ error = connect(data, (struct sockaddr *)_addr,
+ data_addr.su_len);
+ while (error != 0) {
+ if (errno == EINTR) {
+ error = connect_wait(data);
+ continue;
+ }
if (activefallback) {
(void)close(data);
data = -1;
Index: usr.bin/ftp/util.c
===
RCS file: /cvs/src/usr.bin/ftp/util.c,v
retrieving revision 1.80
diff -u -p -u -r1.80 util.c
--- usr.bin/ftp/util.c 18 Aug 2016 16:23:06 - 1.80
+++ usr.bin/ftp/util.c 19 Aug 2016 22:03:10 -
@@ -67,6 +67,7 @@
* FTP User Program -- Misc support routines
*/
#include
+#include
#include
#include
@@ -1070,35 +1071,25 @@ controlediting(void)
#endif /* !SMALL */
/*
- * Wrapper for connect(2) that restarts the syscall when
- * interrupted and operates synchronously.
+ * Wait for an asynchronous connect(2) attempt to finish.
*/
int
-connect_sync(int s, const struct sockaddr *name, socklen_t namelen)
+connect_wait(int s)
{
struct pollfd pfd[1];
int error = 0;
socklen_t len = sizeof(error);
- if (connect(s, name, namelen) < 0) {
- if (errno != EINTR)
- return -1;
- }
-
- /* An interrupted connect(2) continues asyncronously. */
pfd[0].fd = s;
Re: connect_sync
Another option is to use the connect_wait() function I added to EXAMPLES in connect(2). You would only call it if connect(2) returns -1 with errno == EINTR. - todd
