rpki-client http cleanup

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

rpki-client http cleanup

Claudio Jeker
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 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.

--
:wq Claudio

Index: http.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.21
diff -u -p -r1.21 http.c
--- http.c 7 Apr 2021 16:40:38 -0000 1.21
+++ http.c 8 Apr 2021 14:17:01 -0000
@@ -107,9 +107,7 @@ struct http_connection {
  size_t bufsz;
  size_t bufpos;
  size_t id;
- size_t chunksz;
- off_t filesize;
- off_t bytes;
+ off_t iosz;
  int status;
  int redirect_loop;
  int fd;
@@ -132,7 +130,7 @@ size_t tls_ca_size;
 static const char *
 http_info(const char *url)
 {
- static char buf[64];
+ static char buf[80];
 
  if (strnvis(buf, url, sizeof buf, VIS_SAFE) >= (int)sizeof buf) {
  /* overflow, add indicator */
@@ -312,24 +310,6 @@ http_free(struct http_connection *conn)
 }
 
 static int
-http_close(struct http_connection *conn)
-{
- if (conn->tls != NULL) {
- switch (tls_close(conn->tls)) {
- case TLS_WANT_POLLIN:
- return WANT_POLLIN;
- case TLS_WANT_POLLOUT:
- return WANT_POLLOUT;
- case 0:
- case -1:
- break;
- }
- }
-
- return -1;
-}
-
-static int
 http_parse_uri(char *uri, char **ohost, char **oport, char **opath)
 {
  char *host, *port = NULL, *path;
@@ -426,50 +406,12 @@ http_new(size_t id, char *uri, char *mod
 }
 
 static int
-http_redirect(struct http_connection *conn, char *uri)
-{
- char *host, *port, *path;
-
- logx("redirect to %s", http_info(uri));
-
- if (http_parse_uri(uri, &host, &port, &path) == -1) {
- free(uri);
- return -1;
- }
-
- free(conn->url);
- conn->url = uri;
- free(conn->host);
- conn->host = host;
- free(conn->port);
- conn->port = port;
- conn->path = path;
- /* keep modified_since since that is part of the request */
- free(conn->last_modified);
- conn->last_modified = NULL;
- free(conn->buf);
- conn->buf = NULL;
- conn->bufpos = 0;
- conn->bufsz = 0;
- tls_close(conn->tls);
- tls_free(conn->tls);
- conn->tls = NULL;
- close(conn->fd);
- conn->state = STATE_INIT;
-
- /* TODO proxy support (overload of host and port) */
-
- if (http_resolv(conn, host, port) == -1)
- return -1;
-
- return 0;
-}
-
-static int
 http_connect(struct http_connection *conn)
 {
  const char *cause = NULL;
 
+ conn->state = STATE_CONNECT;
+
  if (conn->fd != -1) {
  close(conn->fd);
  conn->fd = -1;
@@ -531,6 +473,7 @@ http_connect(struct http_connection *con
 
  freeaddrinfo(conn->res0);
  conn->res0 = NULL;
+ conn->res = NULL;
 
 #if 0
  /* TODO proxy connect */
@@ -560,6 +503,7 @@ http_finish_connect(struct http_connecti
 
  freeaddrinfo(conn->res0);
  conn->res0 = NULL;
+ conn->res = NULL;
 
 #if 0
  /* TODO proxy connect */
@@ -666,29 +610,6 @@ http_request(struct http_connection *con
 }
 
 static int
-http_write(struct http_connection *conn)
-{
- ssize_t s;
-
- s = tls_write(conn->tls, conn->buf + conn->bufpos,
-    conn->bufsz - conn->bufpos);
- if (s == -1) {
- warnx("%s: TLS write: %s", http_info(conn->url),
-    tls_error(conn->tls));
- return -1;
- } else if (s == TLS_WANT_POLLIN) {
- return WANT_POLLIN;
- } else if (s == TLS_WANT_POLLOUT) {
- return WANT_POLLOUT;
- }
-
- conn->bufpos += s;
- if (conn->bufpos == conn->bufsz)
- return 0;
- return WANT_POLLOUT;
-}
-
-static int
 http_parse_status(struct http_connection *conn, char *buf)
 {
  const char *errstr;
@@ -746,6 +667,46 @@ http_isredirect(struct http_connection *
 }
 
 static int
+http_redirect(struct http_connection *conn, char *uri)
+{
+ char *host, *port, *path;
+
+ logx("redirect to %s", http_info(uri));
+
+ if (http_parse_uri(uri, &host, &port, &path) == -1) {
+ free(uri);
+ return -1;
+ }
+
+ free(conn->url);
+ conn->url = uri;
+ free(conn->host);
+ conn->host = host;
+ free(conn->port);
+ conn->port = port;
+ conn->path = path;
+ /* keep modified_since since that is part of the request */
+ free(conn->last_modified);
+ conn->last_modified = NULL;
+ free(conn->buf);
+ conn->buf = NULL;
+ conn->bufpos = 0;
+ conn->bufsz = 0;
+ tls_close(conn->tls);
+ tls_free(conn->tls);
+ conn->tls = NULL;
+ close(conn->fd);
+ conn->state = STATE_INIT;
+
+ /* TODO proxy support (overload of host and port) */
+
+ if (http_resolv(conn, host, port) == -1)
+ return -1;
+
+ return http_connect(conn);
+}
+
+static int
 http_parse_header(struct http_connection *conn, char *buf)
 {
 #define CONTENTLEN "Content-Length: "
@@ -765,10 +726,10 @@ http_parse_header(struct http_connection
  cp += sizeof(CONTENTLEN) - 1;
  if ((s = strcspn(cp, " \t")) != 0)
  *(cp+s) = 0;
- conn->filesize = strtonum(cp, 0, LLONG_MAX, &errstr);
+ conn->iosz = strtonum(cp, 0, LLONG_MAX, &errstr);
  if (errstr != NULL) {
- warnx("Improper response from %s",
-    http_info(conn->url));
+ warnx("Content-Length of %s is %s",
+    http_info(conn->url), errstr);
  return -1;
  }
  } else if (http_isredirect(conn) &&
@@ -858,7 +819,7 @@ http_parse_chunked(struct http_connectio
 {
  char *header = buf;
  char *end;
- int chunksize;
+ unsigned long chunksize;
 
  /* ignore empty lines, used between chunk and next header */
  if (*header == '\0')
@@ -868,14 +829,14 @@ http_parse_chunked(struct http_connectio
  header[strcspn(header, ";\r\n")] = '\0';
  errno = 0;
  chunksize = strtoul(header, &end, 16);
- if (errno != 0 || header[0] == '\0' || *end != '\0' ||
-    chunksize > INT_MAX) {
+ if (header[0] == '\0' || *end != '\0' || (errno == ERANGE &&
+    chunksize == ULONG_MAX) || chunksize > INT_MAX) {
  warnx("%s: Invalid chunk size", http_info(conn->url));
  return -1;
  }
- conn->chunksz = chunksize;
+ conn->iosz = chunksize;
 
- if (conn->chunksz == 0) {
+ if (conn->iosz == 0) {
  http_done(conn, HTTP_OK);
  return 0;
  }
@@ -933,11 +894,11 @@ http_read(struct http_connection *conn)
  return WANT_POLLIN;
  case STATE_RESPONSE_DATA:
  if (conn->bufpos == conn->bufsz ||
-    conn->filesize - conn->bytes <= (off_t)conn->bufpos)
+    conn->iosz <= (off_t)conn->bufpos)
  return 0;
  return WANT_POLLIN;
  case STATE_RESPONSE_CHUNKED:
- while (conn->chunksz == 0) {
+ while (conn->iosz == 0) {
  buf = http_get_line(conn);
  if (buf == NULL)
  return WANT_POLLIN;
@@ -953,7 +914,7 @@ http_read(struct http_connection *conn)
  }
 
  if (conn->bufpos == conn->bufsz ||
-    conn->chunksz <= conn->bufpos)
+    conn->iosz <= (off_t)conn->bufpos)
  return 0;
  return WANT_POLLIN;
  default:
@@ -962,67 +923,106 @@ http_read(struct http_connection *conn)
 }
 
 static int
-data_write(struct http_connection *conn)
+http_write(struct http_connection *conn)
 {
  ssize_t s;
- size_t bsz = conn->bufpos;
-
- if (conn->filesize - conn->bytes < (off_t)bsz)
- bsz = conn->filesize - conn->bytes;
 
- s = write(conn->outfd, conn->buf, bsz);
+ s = tls_write(conn->tls, conn->buf + conn->bufpos,
+    conn->bufsz - conn->bufpos);
  if (s == -1) {
- warn("%s: Data write", http_info(conn->url));
+ warnx("%s: TLS write: %s", http_info(conn->url),
+    tls_error(conn->tls));
  return -1;
+ } else if (s == TLS_WANT_POLLIN) {
+ return WANT_POLLIN;
+ } else if (s == TLS_WANT_POLLOUT) {
+ return WANT_POLLOUT;
  }
 
- conn->bufpos -= s;
- conn->bytes += s;
- memmove(conn->buf, conn->buf + s, conn->bufpos);
-
- if (conn->bytes == conn->filesize) {
- http_done(conn, HTTP_OK);
+ conn->bufpos += s;
+ if (conn->bufpos == conn->bufsz)
  return 0;
- }
+ return WANT_POLLOUT;
+}
 
- if (conn->bufpos == 0)
- return 0;
+static int
+http_close(struct http_connection *conn)
+{
+ if (conn->tls != NULL) {
+ switch (tls_close(conn->tls)) {
+ case TLS_WANT_POLLIN:
+ return WANT_POLLIN;
+ case TLS_WANT_POLLOUT:
+ return WANT_POLLOUT;
+ case 0:
+ case -1:
+ break;
+ }
+ }
 
- return WANT_POLLOUT;
+ return -1;
 }
 
 static int
-chunk_write(struct http_connection *conn)
+data_write(struct http_connection *conn)
 {
  ssize_t s;
  size_t bsz = conn->bufpos;
 
- if (bsz > conn->chunksz)
- bsz = conn->chunksz;
+ if (conn->iosz < (off_t)bsz)
+ bsz = conn->iosz;
 
  s = write(conn->outfd, conn->buf, bsz);
  if (s == -1) {
- warn("%s: Chunk write", http_info(conn->url));
+ warn("%s: data write", http_info(conn->url));
  return -1;
  }
 
  conn->bufpos -= s;
- conn->chunksz -= s;
- conn->bytes += s;
+ conn->iosz -= s;
  memmove(conn->buf, conn->buf + s, conn->bufpos);
 
- if (conn->bufpos == 0 || conn->chunksz == 0)
+ /* check if regular file transfer is finished */
+ if (conn->iosz == 0 && conn->chunked == 0) {
+ http_done(conn, HTTP_OK);
+ return 0;
+ }
+
+ /* all data written, switch back to read */
+ if (conn->bufpos == 0 || conn->iosz == 0)
  return 0;
 
+ /* still more data to write in buffer */
  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)
 {
+ 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 */
@@ -1038,28 +1038,34 @@ http_handle(struct http_connection *conn
  case STATE_RESPONSE_CHUNKED:
  return http_read(conn);
  case STATE_WRITE_DATA:
- if (conn->chunked)
- return chunk_write(conn);
- else
- return data_write(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);
@@ -1107,9 +1113,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)
@@ -1129,6 +1135,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:
@@ -1219,6 +1228,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 == 0)
+ continue;
+
+ 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;
@@ -1236,23 +1261,11 @@ 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;
  }
  }
- }
- 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)))
- continue;
-
- if (http_do(conn) == -1)
- http_conns[i] = NULL;
  }
  }
 

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client http cleanup

Theo Buehler-3
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.

>
> --
> :wq Claudio
>
> Index: http.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 http.c
> --- http.c 7 Apr 2021 16:40:38 -0000 1.21
> +++ http.c 8 Apr 2021 14:17:01 -0000
> @@ -107,9 +107,7 @@ struct http_connection {
>   size_t bufsz;
>   size_t bufpos;
>   size_t id;
> - size_t chunksz;
> - off_t filesize;
> - off_t bytes;
> + off_t iosz;
>   int status;
>   int redirect_loop;
>   int fd;
> @@ -132,7 +130,7 @@ size_t tls_ca_size;
>  static const char *
>  http_info(const char *url)
>  {
> - static char buf[64];
> + static char buf[80];
>  
>   if (strnvis(buf, url, sizeof buf, VIS_SAFE) >= (int)sizeof buf) {
>   /* overflow, add indicator */
> @@ -312,24 +310,6 @@ http_free(struct http_connection *conn)
>  }
>  
>  static int
> -http_close(struct http_connection *conn)
> -{
> - if (conn->tls != NULL) {
> - switch (tls_close(conn->tls)) {
> - case TLS_WANT_POLLIN:
> - return WANT_POLLIN;
> - case TLS_WANT_POLLOUT:
> - return WANT_POLLOUT;
> - case 0:
> - case -1:
> - break;
> - }
> - }
> -
> - return -1;
> -}
> -
> -static int
>  http_parse_uri(char *uri, char **ohost, char **oport, char **opath)
>  {
>   char *host, *port = NULL, *path;
> @@ -426,50 +406,12 @@ http_new(size_t id, char *uri, char *mod
>  }
>  
>  static int
> -http_redirect(struct http_connection *conn, char *uri)
> -{
> - char *host, *port, *path;
> -
> - logx("redirect to %s", http_info(uri));
> -
> - if (http_parse_uri(uri, &host, &port, &path) == -1) {
> - free(uri);
> - return -1;
> - }
> -
> - free(conn->url);
> - conn->url = uri;
> - free(conn->host);
> - conn->host = host;
> - free(conn->port);
> - conn->port = port;
> - conn->path = path;
> - /* keep modified_since since that is part of the request */
> - free(conn->last_modified);
> - conn->last_modified = NULL;
> - free(conn->buf);
> - conn->buf = NULL;
> - conn->bufpos = 0;
> - conn->bufsz = 0;
> - tls_close(conn->tls);
> - tls_free(conn->tls);
> - conn->tls = NULL;
> - close(conn->fd);
> - conn->state = STATE_INIT;
> -
> - /* TODO proxy support (overload of host and port) */
> -
> - if (http_resolv(conn, host, port) == -1)
> - return -1;
> -
> - return 0;
> -}
> -
> -static int
>  http_connect(struct http_connection *conn)
>  {
>   const char *cause = NULL;
>  
> + conn->state = STATE_CONNECT;
> +
>   if (conn->fd != -1) {
>   close(conn->fd);
>   conn->fd = -1;
> @@ -531,6 +473,7 @@ http_connect(struct http_connection *con
>  
>   freeaddrinfo(conn->res0);
>   conn->res0 = NULL;
> + conn->res = NULL;

A few lines before this, there's another freeaddrinfo() where you don't
NULL out conn->res. I don't think it matters since you'll promptly end
in the case -1: of http_do(), but I would still do it for consistency.

>  
>  #if 0
>   /* TODO proxy connect */
> @@ -560,6 +503,7 @@ http_finish_connect(struct http_connecti
>  
>   freeaddrinfo(conn->res0);
>   conn->res0 = NULL;
> + conn->res = NULL;
>  
>  #if 0
>   /* TODO proxy connect */
> @@ -666,29 +610,6 @@ http_request(struct http_connection *con
>  }
>  
>  static int
> -http_write(struct http_connection *conn)
> -{
> - ssize_t s;
> -
> - s = tls_write(conn->tls, conn->buf + conn->bufpos,
> -    conn->bufsz - conn->bufpos);
> - if (s == -1) {
> - warnx("%s: TLS write: %s", http_info(conn->url),
> -    tls_error(conn->tls));
> - return -1;
> - } else if (s == TLS_WANT_POLLIN) {
> - return WANT_POLLIN;
> - } else if (s == TLS_WANT_POLLOUT) {
> - return WANT_POLLOUT;
> - }
> -
> - conn->bufpos += s;
> - if (conn->bufpos == conn->bufsz)
> - return 0;
> - return WANT_POLLOUT;
> -}
> -
> -static int
>  http_parse_status(struct http_connection *conn, char *buf)
>  {
>   const char *errstr;
> @@ -746,6 +667,46 @@ http_isredirect(struct http_connection *
>  }
>  
>  static int
> +http_redirect(struct http_connection *conn, char *uri)
> +{
> + char *host, *port, *path;
> +
> + logx("redirect to %s", http_info(uri));
> +
> + if (http_parse_uri(uri, &host, &port, &path) == -1) {
> + free(uri);
> + return -1;
> + }
> +
> + free(conn->url);
> + conn->url = uri;
> + free(conn->host);
> + conn->host = host;
> + free(conn->port);
> + conn->port = port;
> + conn->path = path;
> + /* keep modified_since since that is part of the request */
> + free(conn->last_modified);
> + conn->last_modified = NULL;
> + free(conn->buf);
> + conn->buf = NULL;
> + conn->bufpos = 0;
> + conn->bufsz = 0;
> + tls_close(conn->tls);
> + tls_free(conn->tls);
> + conn->tls = NULL;
> + close(conn->fd);
> + conn->state = STATE_INIT;
> +
> + /* TODO proxy support (overload of host and port) */
> +
> + if (http_resolv(conn, host, port) == -1)
> + return -1;
> +
> + return http_connect(conn);
> +}
> +
> +static int
>  http_parse_header(struct http_connection *conn, char *buf)
>  {
>  #define CONTENTLEN "Content-Length: "
> @@ -765,10 +726,10 @@ http_parse_header(struct http_connection
>   cp += sizeof(CONTENTLEN) - 1;
>   if ((s = strcspn(cp, " \t")) != 0)
>   *(cp+s) = 0;
> - conn->filesize = strtonum(cp, 0, LLONG_MAX, &errstr);
> + conn->iosz = strtonum(cp, 0, LLONG_MAX, &errstr);
>   if (errstr != NULL) {
> - warnx("Improper response from %s",
> -    http_info(conn->url));
> + warnx("Content-Length of %s is %s",
> +    http_info(conn->url), errstr);
>   return -1;
>   }
>   } else if (http_isredirect(conn) &&
> @@ -858,7 +819,7 @@ http_parse_chunked(struct http_connectio
>  {
>   char *header = buf;
>   char *end;
> - int chunksize;
> + unsigned long chunksize;
>  
>   /* ignore empty lines, used between chunk and next header */
>   if (*header == '\0')
> @@ -868,14 +829,14 @@ http_parse_chunked(struct http_connectio
>   header[strcspn(header, ";\r\n")] = '\0';
>   errno = 0;
>   chunksize = strtoul(header, &end, 16);
> - if (errno != 0 || header[0] == '\0' || *end != '\0' ||
> -    chunksize > INT_MAX) {
> + if (header[0] == '\0' || *end != '\0' || (errno == ERANGE &&
> +    chunksize == ULONG_MAX) || chunksize > INT_MAX) {
>   warnx("%s: Invalid chunk size", http_info(conn->url));
>   return -1;
>   }
> - conn->chunksz = chunksize;
> + conn->iosz = chunksize;
>  
> - if (conn->chunksz == 0) {
> + if (conn->iosz == 0) {
>   http_done(conn, HTTP_OK);
>   return 0;
>   }
> @@ -933,11 +894,11 @@ http_read(struct http_connection *conn)
>   return WANT_POLLIN;
>   case STATE_RESPONSE_DATA:
>   if (conn->bufpos == conn->bufsz ||
> -    conn->filesize - conn->bytes <= (off_t)conn->bufpos)
> +    conn->iosz <= (off_t)conn->bufpos)
>   return 0;
>   return WANT_POLLIN;
>   case STATE_RESPONSE_CHUNKED:
> - while (conn->chunksz == 0) {
> + while (conn->iosz == 0) {
>   buf = http_get_line(conn);
>   if (buf == NULL)
>   return WANT_POLLIN;
> @@ -953,7 +914,7 @@ http_read(struct http_connection *conn)
>   }
>  
>   if (conn->bufpos == conn->bufsz ||
> -    conn->chunksz <= conn->bufpos)
> +    conn->iosz <= (off_t)conn->bufpos)
>   return 0;
>   return WANT_POLLIN;
>   default:
> @@ -962,67 +923,106 @@ http_read(struct http_connection *conn)
>  }
>  
>  static int
> -data_write(struct http_connection *conn)
> +http_write(struct http_connection *conn)
>  {
>   ssize_t s;
> - size_t bsz = conn->bufpos;
> -
> - if (conn->filesize - conn->bytes < (off_t)bsz)
> - bsz = conn->filesize - conn->bytes;
>  
> - s = write(conn->outfd, conn->buf, bsz);
> + s = tls_write(conn->tls, conn->buf + conn->bufpos,
> +    conn->bufsz - conn->bufpos);
>   if (s == -1) {
> - warn("%s: Data write", http_info(conn->url));
> + warnx("%s: TLS write: %s", http_info(conn->url),
> +    tls_error(conn->tls));
>   return -1;
> + } else if (s == TLS_WANT_POLLIN) {
> + return WANT_POLLIN;
> + } else if (s == TLS_WANT_POLLOUT) {
> + return WANT_POLLOUT;
>   }
>  
> - conn->bufpos -= s;
> - conn->bytes += s;
> - memmove(conn->buf, conn->buf + s, conn->bufpos);
> -
> - if (conn->bytes == conn->filesize) {
> - http_done(conn, HTTP_OK);
> + conn->bufpos += s;
> + if (conn->bufpos == conn->bufsz)
>   return 0;
> - }
> + return WANT_POLLOUT;
> +}
>  
> - if (conn->bufpos == 0)
> - return 0;
> +static int
> +http_close(struct http_connection *conn)
> +{
> + if (conn->tls != NULL) {
> + switch (tls_close(conn->tls)) {
> + case TLS_WANT_POLLIN:
> + return WANT_POLLIN;
> + case TLS_WANT_POLLOUT:
> + return WANT_POLLOUT;
> + case 0:
> + case -1:
> + break;
> + }
> + }
>  
> - return WANT_POLLOUT;
> + return -1;
>  }
>  
>  static int
> -chunk_write(struct http_connection *conn)
> +data_write(struct http_connection *conn)
>  {
>   ssize_t s;
>   size_t bsz = conn->bufpos;
>  
> - if (bsz > conn->chunksz)
> - bsz = conn->chunksz;
> + if (conn->iosz < (off_t)bsz)
> + bsz = conn->iosz;
>  
>   s = write(conn->outfd, conn->buf, bsz);
>   if (s == -1) {
> - warn("%s: Chunk write", http_info(conn->url));
> + warn("%s: data write", http_info(conn->url));
>   return -1;
>   }
>  
>   conn->bufpos -= s;
> - conn->chunksz -= s;
> - conn->bytes += s;
> + conn->iosz -= s;
>   memmove(conn->buf, conn->buf + s, conn->bufpos);
>  
> - if (conn->bufpos == 0 || conn->chunksz == 0)
> + /* check if regular file transfer is finished */
> + if (conn->iosz == 0 && conn->chunked == 0) {
> + http_done(conn, HTTP_OK);
> + return 0;
> + }
> +
> + /* all data written, switch back to read */
> + if (conn->bufpos == 0 || conn->iosz == 0)
>   return 0;
>  
> + /* still more data to write in buffer */
>   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)
>  {
> + 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 */
> @@ -1038,28 +1038,34 @@ http_handle(struct http_connection *conn
>   case STATE_RESPONSE_CHUNKED:
>   return http_read(conn);
>   case STATE_WRITE_DATA:
> - if (conn->chunked)
> - return chunk_write(conn);
> - else
> - return data_write(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);
> @@ -1107,9 +1113,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)
> @@ -1129,6 +1135,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:
> @@ -1219,6 +1228,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 == 0)
> + continue;
> +
> + 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;
> @@ -1236,23 +1261,11 @@ 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;
>   }
>   }
> - }
> - 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)))
> - continue;
> -
> - if (http_do(conn) == -1)
> - http_conns[i] = NULL;
>   }
>   }
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client http cleanup

Claudio Jeker
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.

--
:wq Claudio

Index: http.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.24
diff -u -p -r1.24 http.c
--- http.c 8 Apr 2021 16:56:34 -0000 1.24
+++ http.c 8 Apr 2021 17:12:49 -0000
@@ -107,9 +107,7 @@ struct http_connection {
  size_t bufsz;
  size_t bufpos;
  size_t id;
- size_t chunksz;
- off_t filesize;
- off_t bytes;
+ off_t iosz;
  int status;
  int redirect_loop;
  int fd;
@@ -727,10 +725,10 @@ http_parse_header(struct http_connection
  cp += sizeof(CONTENTLEN) - 1;
  if ((s = strcspn(cp, " \t")) != 0)
  *(cp+s) = 0;
- conn->filesize = strtonum(cp, 0, LLONG_MAX, &errstr);
+ conn->iosz = strtonum(cp, 0, LLONG_MAX, &errstr);
  if (errstr != NULL) {
- warnx("Improper response from %s",
-    http_info(conn->url));
+ warnx("Content-Length of %s is %s",
+    http_info(conn->url), errstr);
  return -1;
  }
  } else if (http_isredirect(conn) &&
@@ -820,7 +818,7 @@ http_parse_chunked(struct http_connectio
 {
  char *header = buf;
  char *end;
- int chunksize;
+ unsigned long chunksize;
 
  /* ignore empty lines, used between chunk and next header */
  if (*header == '\0')
@@ -830,14 +828,14 @@ http_parse_chunked(struct http_connectio
  header[strcspn(header, ";\r\n")] = '\0';
  errno = 0;
  chunksize = strtoul(header, &end, 16);
- if (errno != 0 || header[0] == '\0' || *end != '\0' ||
-    chunksize > INT_MAX) {
+ if (header[0] == '\0' || *end != '\0' || (errno == ERANGE &&
+    chunksize == ULONG_MAX) || chunksize > INT_MAX) {
  warnx("%s: Invalid chunk size", http_info(conn->url));
  return -1;
  }
- conn->chunksz = chunksize;
+ conn->iosz = chunksize;
 
- if (conn->chunksz == 0) {
+ if (conn->iosz == 0) {
  http_done(conn, HTTP_OK);
  return 0;
  }
@@ -895,11 +893,11 @@ http_read(struct http_connection *conn)
  return WANT_POLLIN;
  case STATE_RESPONSE_DATA:
  if (conn->bufpos == conn->bufsz ||
-    conn->filesize - conn->bytes <= (off_t)conn->bufpos)
+    conn->iosz <= (off_t)conn->bufpos)
  return 0;
  return WANT_POLLIN;
  case STATE_RESPONSE_CHUNKED:
- while (conn->chunksz == 0) {
+ while (conn->iosz == 0) {
  buf = http_get_line(conn);
  if (buf == NULL)
  return WANT_POLLIN;
@@ -915,7 +913,7 @@ http_read(struct http_connection *conn)
  }
 
  if (conn->bufpos == conn->bufsz ||
-    conn->chunksz <= conn->bufpos)
+    conn->iosz <= (off_t)conn->bufpos)
  return 0;
  return WANT_POLLIN;
  default:
@@ -970,53 +968,30 @@ data_write(struct http_connection *conn)
  ssize_t s;
  size_t bsz = conn->bufpos;
 
- if (conn->filesize - conn->bytes < (off_t)bsz)
- bsz = conn->filesize - conn->bytes;
+ if (conn->iosz < (off_t)bsz)
+ bsz = conn->iosz;
 
  s = write(conn->outfd, conn->buf, bsz);
  if (s == -1) {
- warn("%s: Data write", http_info(conn->url));
+ warn("%s: data write", http_info(conn->url));
  return -1;
  }
 
  conn->bufpos -= s;
- conn->bytes += s;
+ conn->iosz -= s;
  memmove(conn->buf, conn->buf + s, conn->bufpos);
 
- if (conn->bytes == conn->filesize) {
+ /* check if regular file transfer is finished */
+ if (conn->iosz == 0 && conn->chunked == 0) {
  http_done(conn, HTTP_OK);
  return 0;
  }
 
- if (conn->bufpos == 0)
- return 0;
-
- return WANT_POLLOUT;
-}
-
-static int
-chunk_write(struct http_connection *conn)
-{
- ssize_t s;
- size_t bsz = conn->bufpos;
-
- if (bsz > conn->chunksz)
- bsz = conn->chunksz;
-
- s = write(conn->outfd, conn->buf, bsz);
- if (s == -1) {
- warn("%s: Chunk write", http_info(conn->url));
- return -1;
- }
-
- conn->bufpos -= s;
- conn->chunksz -= s;
- conn->bytes += s;
- memmove(conn->buf, conn->buf + s, conn->bufpos);
-
- if (conn->bufpos == 0 || conn->chunksz == 0)
+ /* all data written, switch back to read */
+ if (conn->bufpos == 0 || conn->iosz == 0)
  return 0;
 
+ /* still more data to write in buffer */
  return WANT_POLLOUT;
 }
 
@@ -1041,10 +1016,7 @@ http_handle(struct http_connection *conn
  case STATE_RESPONSE_CHUNKED:
  return http_read(conn);
  case STATE_WRITE_DATA:
- if (conn->chunked)
- return chunk_write(conn);
- else
- return data_write(conn);
+ return data_write(conn);
  case STATE_DONE:
  return http_close(conn);
  case STATE_FREE:

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client http cleanup

Theo Buehler-3
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.

>
> --
> :wq Claudio
>
> Index: http.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 http.c
> --- http.c 8 Apr 2021 16:56:34 -0000 1.24
> +++ http.c 8 Apr 2021 17:12:49 -0000
> @@ -107,9 +107,7 @@ struct http_connection {
>   size_t bufsz;
>   size_t bufpos;
>   size_t id;
> - size_t chunksz;
> - off_t filesize;
> - off_t bytes;
> + off_t iosz;
>   int status;
>   int redirect_loop;
>   int fd;
> @@ -727,10 +725,10 @@ http_parse_header(struct http_connection
>   cp += sizeof(CONTENTLEN) - 1;
>   if ((s = strcspn(cp, " \t")) != 0)
>   *(cp+s) = 0;
> - conn->filesize = strtonum(cp, 0, LLONG_MAX, &errstr);
> + conn->iosz = strtonum(cp, 0, LLONG_MAX, &errstr);
>   if (errstr != NULL) {
> - warnx("Improper response from %s",
> -    http_info(conn->url));
> + warnx("Content-Length of %s is %s",
> +    http_info(conn->url), errstr);
>   return -1;
>   }
>   } else if (http_isredirect(conn) &&
> @@ -820,7 +818,7 @@ http_parse_chunked(struct http_connectio
>  {
>   char *header = buf;
>   char *end;
> - int chunksize;
> + unsigned long chunksize;
>  
>   /* ignore empty lines, used between chunk and next header */
>   if (*header == '\0')
> @@ -830,14 +828,14 @@ http_parse_chunked(struct http_connectio
>   header[strcspn(header, ";\r\n")] = '\0';
>   errno = 0;
>   chunksize = strtoul(header, &end, 16);
> - if (errno != 0 || header[0] == '\0' || *end != '\0' ||
> -    chunksize > INT_MAX) {
> + if (header[0] == '\0' || *end != '\0' || (errno == ERANGE &&
> +    chunksize == ULONG_MAX) || chunksize > INT_MAX) {
>   warnx("%s: Invalid chunk size", http_info(conn->url));
>   return -1;
>   }
> - conn->chunksz = chunksize;
> + conn->iosz = chunksize;
>  
> - if (conn->chunksz == 0) {
> + if (conn->iosz == 0) {
>   http_done(conn, HTTP_OK);
>   return 0;
>   }
> @@ -895,11 +893,11 @@ http_read(struct http_connection *conn)
>   return WANT_POLLIN;
>   case STATE_RESPONSE_DATA:
>   if (conn->bufpos == conn->bufsz ||
> -    conn->filesize - conn->bytes <= (off_t)conn->bufpos)
> +    conn->iosz <= (off_t)conn->bufpos)
>   return 0;
>   return WANT_POLLIN;
>   case STATE_RESPONSE_CHUNKED:
> - while (conn->chunksz == 0) {
> + while (conn->iosz == 0) {
>   buf = http_get_line(conn);
>   if (buf == NULL)
>   return WANT_POLLIN;
> @@ -915,7 +913,7 @@ http_read(struct http_connection *conn)
>   }
>  
>   if (conn->bufpos == conn->bufsz ||
> -    conn->chunksz <= conn->bufpos)
> +    conn->iosz <= (off_t)conn->bufpos)
>   return 0;
>   return WANT_POLLIN;
>   default:
> @@ -970,53 +968,30 @@ data_write(struct http_connection *conn)
>   ssize_t s;
>   size_t bsz = conn->bufpos;
>  
> - if (conn->filesize - conn->bytes < (off_t)bsz)
> - bsz = conn->filesize - conn->bytes;
> + if (conn->iosz < (off_t)bsz)
> + bsz = conn->iosz;
>  
>   s = write(conn->outfd, conn->buf, bsz);
>   if (s == -1) {
> - warn("%s: Data write", http_info(conn->url));
> + warn("%s: data write", http_info(conn->url));
>   return -1;
>   }
>  
>   conn->bufpos -= s;
> - conn->bytes += s;
> + conn->iosz -= s;
>   memmove(conn->buf, conn->buf + s, conn->bufpos);
>  
> - if (conn->bytes == conn->filesize) {
> + /* check if regular file transfer is finished */
> + if (conn->iosz == 0 && conn->chunked == 0) {

I would treat chunked as a Boolean (as is done elsewhere) and perhaps
switch the logic around (matches the comment better):

        if (!conn->chunked && conn->iosz == 0) {

>   http_done(conn, HTTP_OK);
>   return 0;
>   }
>  
> - if (conn->bufpos == 0)
> - return 0;
> -
> - return WANT_POLLOUT;
> -}
> -
> -static int
> -chunk_write(struct http_connection *conn)
> -{
> - ssize_t s;
> - size_t bsz = conn->bufpos;
> -
> - if (bsz > conn->chunksz)
> - bsz = conn->chunksz;
> -
> - s = write(conn->outfd, conn->buf, bsz);
> - if (s == -1) {
> - warn("%s: Chunk write", http_info(conn->url));
> - return -1;
> - }
> -
> - conn->bufpos -= s;
> - conn->chunksz -= s;
> - conn->bytes += s;
> - memmove(conn->buf, conn->buf + s, conn->bufpos);
> -
> - if (conn->bufpos == 0 || conn->chunksz == 0)
> + /* all data written, switch back to read */
> + if (conn->bufpos == 0 || conn->iosz == 0)
>   return 0;
>  
> + /* still more data to write in buffer */
>   return WANT_POLLOUT;
>  }
>  
> @@ -1041,10 +1016,7 @@ http_handle(struct http_connection *conn
>   case STATE_RESPONSE_CHUNKED:
>   return http_read(conn);
>   case STATE_WRITE_DATA:
> - if (conn->chunked)
> - return chunk_write(conn);
> - else
> - return data_write(conn);
> + return data_write(conn);
>   case STATE_DONE:
>   return http_close(conn);
>   case STATE_FREE:

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client http cleanup

Claudio Jeker
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;
  }

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client http cleanup

Theo Buehler-3
On Thu, Apr 08, 2021 at 08:43:25PM +0200, Claudio Jeker wrote:
> 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.
>

ok tb

> --
> :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;
>   }