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:
