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:
