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

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

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


Patch Set 4: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/2932/4/include/asterisk/iostream.h
File include/asterisk/iostream.h:

PS4, Line 103: int ast_iostream_get_fd(struct ast_iostream *stream);
             : void ast_iostream_nonblock(struct ast_iostream *stream);
             : 
             : SSL* ast_iostream_get_ssl(struct ast_iostream *stream);
             : 
             : ssize_t ast_iostream_read(struct ast_iostream *stream, void *buf, size_t count);
             : ssize_t ast_iostream_gets(struct ast_iostream *stream, char *buf, size_t count);
             : ssize_t ast_iostream_discard(struct ast_iostream *stream, size_t count);
             : ssize_t ast_iostream_write(struct ast_iostream *stream, const void *buf, size_t count);
             : 
             : struct ast_iostream* ast_iostream_from_fd(int *fd);
             : int ast_iostream_start_tls(struct ast_iostream **stream, SSL_CTX *ctx, int client);
             : int ast_iostream_close(struct ast_iostream *stream);
I know some of these functions are just copied over, but I think it would be good to go ahead and add documentation for them.


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

PS4, Line 219: 		memcpy(buf, stream->rbuf, r);
             : 		stream->rbuflen -= r;
             : 		if (stream->rbuflen) {
             : 			memmove(stream->rbuf, &stream->rbuf[r], stream->rbuflen);
             : 		}
If r > rbuflen there is going to be some problems here. memcpy will try to copy more bytes than available. memove will try to move negative bytes into a negatively index location.


PS4, Line 214: 	if (stream->rbuflen) {
             : 		size_t r = size;
             : 		if (stream->rbuflen < r) {
             : 			r = stream->rbuflen;
             : 		}
             : 		memcpy(buf, stream->rbuf, r);
             : 		stream->rbuflen -= r;
             : 		if (stream->rbuflen) {
             : 			memmove(stream->rbuf, &stream->rbuf[r], stream->rbuflen);
             : 		}
             : 		return r;
             : 	}
I think if size > rbuflen then this should read up to 'size' bytes from the read buffer and then read the rest (up to size) from the fd.


Line 230: ssize_t ast_iostream_gets(struct ast_iostream *stream, char *buf, size_t count)
I'd suggest reworking this a bit and making a more generic function that can accept a character delimiter of any kind. Then use macros/functions to default whether the function reads one byte ('getchar'), all bytes up to newline ('getchars'), etc...

If count/size is not given (=0) then read up to some default 'n' (or up to a delimiter if given) bytes storing the rest in the rbuf.

'ast_iostream_read' would also just call this function using an empty delimiter now essentially eliminating the need for that code as well.


PS4, Line 277: 	while (remaining) {
             : 		ret = ast_iostream_read(stream, buf, remaining > sizeof(buf) ? sizeof(buf) : remaining);
             : 		if (ret < 0) {
             : 			return ret;
             : 		}
             : 		remaining -= ret;
             : 	}
What about any data that is in the rbuf?


Line 288: ssize_t ast_iostream_write(struct ast_iostream *stream, const void *buf, size_t size)
Any thoughts on having a wbuf (write buffer) as well along with a flush function?


Line 401: int ast_iostream_close(struct ast_iostream *stream)
Would it make sense to move all the code for this function into the destructor? So when the stream's ref count goes to zero it is just automatically ran, or are there cases where an explicit call to 'close' makes sense?


Line 483: 	stream = tcptls_stream_alloc();
Pass 'fd' in to the alloc function and initialize is there. This function should then collapse to a single line.


Line 518: 	} 
extra space/red blob


-- 
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: 4
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Timo Teräs <timo.teras at iki.fi>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes