On Thu, Apr 08, 2021 at 07:43:47PM +0200, Theo Buehler wrote:
> On Thu, Apr 08, 2021 at 07:18:39PM +0200, Claudio Jeker wrote:
> > On Thu, Apr 08, 2021 at 06:22:16PM +0200, Theo Buehler wrote:
> > > On Thu, Apr 08, 2021 at 04:47:15PM +0200, Claudio Jeker wrote:
> > > > This diff is a first step in tightening the code in http.c
> > > > It should cleanup the poll handling and make adds some code to ensure
> > > > that
> > > > only expected results are returned. The goal is that http_handle() only
> > > > does IO processing and http_nextstep() is used for transitions into new
> > > > states.
> > >
> > > I like this direction.
> > >
> > > > I did shuffle the code around a bit. So that similar functions are
> > > > together.
> > > > Also by shuffling the poll loop now connections are added after the all
> > > > active connections have been processed.
> > >
> > > This added quite a bit of noise to the diff (especially some parts
> > > shuffling _write() functions are hard to read).
> > >
> > > I'm ok with this, one small comment below.
> > >
> >
> > Sorry about that. I committed the deck chair shuffle and some other minor
> > bits. Here is the part that replaces the regular and chunked write
> > functions with a single data_write one.
> >
> > I made the strtoul() check look like the man page example to detect
> > overflows. There is some off_t vs size_t casts required but in that case
> > the size limited bufpos is casted.
>
>
> > On Thu, Apr 08, 2021 at 06:22:16PM +0200, Theo Buehler wrote:
> > > On Thu, Apr 08, 2021 at 04:47:15PM +0200, Claudio Jeker wrote:
> > > > This diff is a first step in tightening the code in http.c
> > > > It should cleanup the poll handling and make adds some code to ensure
> > > > that
> > > > only expected results are returned. The goal is that http_handle() only
> > > > does IO processing and http_nextstep() is used for transitions into new
> > > > states.
> > >
> > > I like this direction.
> > >
> > > > I did shuffle the code around a bit. So that similar functions are
> > > > together.
> > > > Also by shuffling the poll loop now connections are added after the all
> > > > active connections have been processed.
> > >
> > > This added quite a bit of noise to the diff (especially some parts
> > > shuffling _write() functions are hard to read).
> > >
> > > I'm ok with this, one small comment below.
> > >
> >
> > Sorry about that. I committed the deck chair shuffle and some other minor
> > bits. Here is the part that replaces the regular and chunked write
> > functions with a single data_write one.
> >
> > I made the strtoul() check look like the man page example to detect
> > overflows. There is some off_t vs size_t casts required but in that case
> > the size limited bufpos is casted.
>
> Thanks. Still looks good. Tiny suggestion below, but ok either way.
Yes, your right. I over optimised it a bit and triggered on the less
common iosz == 0 first.
Also here is the last bit of the http work. This changes http_handle() and
ensures that http_nextstep() never returns 0. For http_tls_connect() this
requires a fall through to the next stage in case it returns 0.
Also http_redirect() now calls http_connect() directly and no longer
requires an extra state machine step.
--
:wq Claudio
? obj
Index: http.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.26
diff -u -p -r1.26 http.c
--- http.c 8 Apr 2021 18:35:02 -0000 1.26
+++ http.c 8 Apr 2021 18:38:22 -0000
@@ -410,6 +410,8 @@ http_connect(struct http_connection *con
{
const char *cause = NULL;
+ conn->state = STATE_CONNECT;
+
if (conn->fd != -1) {
close(conn->fd);
conn->fd = -1;
@@ -702,7 +704,7 @@ http_redirect(struct http_connection *co
if (http_resolv(conn, host, port) == -1)
return -1;
- return 0;
+ return http_connect(conn);
}
static int
@@ -995,12 +997,34 @@ data_write(struct http_connection *conn)
return WANT_POLLOUT;
}
+/*
+ * Do one IO call depending on the connection state.
+ * Return WANT_POLLIN or WANT_POLLOUT to poll for more data.
+ * If 0 is returned this stage is finished and the protocol should move
+ * to the next stage by calling http_nextstep(). On error return -1.
+ */
static int
-http_handle(struct http_connection *conn)
+http_handle(struct http_connection *conn, int events)
{
+ /* special case for INIT, there is no event to start a request */
+ if (conn->state == STATE_INIT)
+ return http_connect(conn);
+
+ if (events & POLLHUP) {
+ if (conn->state == STATE_WRITE_DATA)
+ warnx("%s: output pipe closed", http_info(conn->url));
+ else
+ warnx("%s: server closed connection",
+ http_info(conn->url));
+ return -1;
+ }
+
+ if ((events & conn->events) == 0) {
+ warnx("%s: unexpected event", http_info(conn->url));
+ return -1;
+ }
+
switch (conn->state) {
- case STATE_INIT:
- return 0;
case STATE_CONNECT:
if (http_finish_connect(conn) == -1)
/* something went wrong, try other host */
@@ -1019,22 +1043,31 @@ http_handle(struct http_connection *conn
return data_write(conn);
case STATE_DONE:
return http_close(conn);
+ case STATE_INIT:
case STATE_FREE:
errx(1, "bad http state");
}
errx(1, "unknown http state");
}
+/*
+ * Move the state machine forward until IO needs to happen.
+ * Returns either WANT_POLLIN or WANT_POLLOUT or -1 on error.
+ */
static int
http_nextstep(struct http_connection *conn)
{
+ int r;
+
switch (conn->state) {
case STATE_INIT:
- conn->state = STATE_CONNECT;
- return http_connect(conn);
+ errx(1, "bad http state");
case STATE_CONNECT:
conn->state = STATE_TLSCONNECT;
- return http_tls_connect(conn);
+ r = http_tls_connect(conn);
+ if (r != 0)
+ return r;
+ /* FALLTHROUGH */
case STATE_TLSCONNECT:
conn->state = STATE_REQUEST;
return http_request(conn);
@@ -1082,9 +1115,9 @@ http_nextstep(struct http_connection *co
}
static int
-http_do(struct http_connection *conn)
+http_do(struct http_connection *conn, int events)
{
- switch (http_handle(conn)) {
+ switch (http_handle(conn, events)) {
case -1:
/* connection failure */
if (conn->state != STATE_DONE)
@@ -1104,6 +1137,9 @@ http_do(struct http_connection *conn)
http_fail(conn->id);
http_free(conn);
return -1;
+ case 0:
+ errx(1, "%s: http_nextstep returned 0, state %d",
+ http_info(conn->url), conn->state);
}
break;
case WANT_POLLIN:
@@ -1194,18 +1230,22 @@ proc_http(char *bind_addr, int fd)
err(1, "write");
}
}
+
+ /* process active http requests */
for (i = 0; i < MAX_CONNECTIONS; i++) {
struct http_connection *conn = http_conns[i];
if (conn == NULL)
continue;
/* event not ready */
- if (!(pfds[i].revents & (conn->events | POLLHUP)))
+ if (pfds[i].revents == 0)
continue;
- if (http_do(conn) == -1)
+ if (http_do(conn, pfds[i].revents) == -1)
http_conns[i] = NULL;
}
+
+ /* process new requests last */
if (pfds[MAX_CONNECTIONS].revents & POLLIN) {
struct http_connection *h;
size_t id;
@@ -1223,7 +1263,7 @@ proc_http(char *bind_addr, int fd)
if (http_conns[i] != NULL)
continue;
http_conns[i] = h;
- if (http_do(h) == -1)
+ if (http_do(h, 0) == -1)
http_conns[i] = NULL;
break;
}
