[Asterisk-code-review] Implement internal abstraction for iostreams (asterisk[master])

[Timo Teräs]

Timo Teräs has posted comments on this change. ( https://gerrit.asterisk.org/2932 )

Change subject: Implement internal abstraction for iostreams
......................................................................


Patch Set 5:

(6 comments)

Fixed other cases than the possible snprintf truncations. Waiting feedback which of the options to implement. Thanks.

https://gerrit.asterisk.org/#/c/2932/5/apps/app_externalivr.c
File apps/app_externalivr.c:

PS5, Line 169: 	ast_str_append(&tmp, 0, "\n");
             : 	ast_iostream_write(stream, ast_str_buffer(tmp), strlen(ast_str_buffer(tmp)));
             : 
             : 	ast_debug(1, "sent '%s'", ast_str_buffer(tmp));
> This edit has a sliiight problem to it. The debug message goes from looking
Fixed by truncating the trailing new-line off.


https://gerrit.asterisk.org/#/c/2932/5/main/http.c
File main/http.c:

PS5, Line 510: 	len = snprintf(buf, sizeof(buf),
             : 		"HTTP/1.1 %d %s\r\n"
             : 		"%s"
             : 		"Date: %s\r\n"
             : 		"%s"
             : 		"%s"
             : 		"%s"
             : 		"Content-Length: %d\r\n"
             : 		"\r\n",
             : 		status_code, status_title ? status_title : "OK",
             : 		ast_str_buffer(server_header_field),
             : 		timebuf,
             : 		close_connection ? "Connection: close\r\n" : "",
             : 		static_content ? "" : "Cache-Control: no-cache, no-store\r\n",
             : 		http_header ? ast_str_buffer(http_header) : "",
             : 		content_length
             : 		);
             : 	ast_iostream_write(ser->stream, buf, len);
> Two potential problems here:
3) Use asprintf() ?
4) Prove that buf is longer than any possible combination of the header lines.

IIRC, the http headers looked like there would be no truncation. But I did not do the exact math.

I tried to avoid not implementing printf like API. But it would give a powerful API, and would be probably simple to implement with asprintf() (or perhaps have a decent stack buffer, use snprintf and if that fails retry with dynamically allocated buffer).

I would probably opt to #2 since there are already three cases which would become trivial to fix if the helper is done.

Any other thoughts for this?


PS5, Line 1060: 	res = ast_iostream_read(ser->stream, chunk_sync, sizeof(chunk_sync));
              : 	if (res < sizeof(chunk_sync)) {
              : 		ast_log(LOG_WARNING, "Short HTTP chunk sync read (Wanted %zu)\n",
              : 			sizeof(chunk_sync));
              : 		return -1;
              : 	}
> The if logic has been altered here. This may actually be correct with the c
The function call return values are different. fread() returns number of successfully read items. In the old code the item length is sizeof chunk_sync, so it returns 1 on success. ast_iostream_read always returns byte value, so it's correctly translated to compare against fully read sync frame.


https://gerrit.asterisk.org/#/c/2932/5/main/iostream.c
File main/iostream.c:

PS5, Line 257: 		r = iostream_read(stream, stream->rbufhead, sizeof(stream->rbuf) - stream->rbuflen);
             : 		if (r <= 0) {
             : 			return r;
             : 		}
> Should the second argument to iostream_read() be
Good catch! Fixed.


PS5, Line 455: static void iostream_dtor(void *cookie)
             : {
             : #ifdef AST_DEVMODE
             : 	/* Since the ast_assert below is the only one using stream,
             : 	 * and ast_assert is only available with AST_DEVMODE, we
             : 	 * put this in a conditional to avoid compiler warnings. */
             : 	struct ast_iostream *stream = cookie;
             : #endif
             : 
             : 	ast_assert(stream->fd == -1);
             : }
> Only the C preprocessor evaluates macro parameters for string substitution.
This is remnant from original code. I just took how it's there. As noted, it works, but is slightly awkward. As comment says, it's intention is to remove compiler warning when ast_assert is not evaluated.


Line 509: 	} 
> Extra trailing space on this line.
Fixed.


-- 
To view, visit https://gerrit.asterisk.org/2932
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id916aef418b665ced6a7489aef74908b6e376e85
Gerrit-PatchSet: 5
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Timo Teräs <timo.teras at iki.fi>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Timo Teräs <timo.teras at iki.fi>
Gerrit-HasComments: Yes