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

[Timo Teräs]

Timo Teräs has posted comments on this change.

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


Patch Set 4:

(7 comments)

Thanks for review comments. Much appreciated. See comments for many of the points.

Additionally, as this is rather intrusive change, it's good to pay extra attention to the timeout handling. I originally made the iostream owned fd unconditionally O_NONBLOCK, but it broken http listening sockets and connect() in some places. I hope all callers now explicitly set O_NONBLOCK mode before reading, as the read loop does not work properly without it.

And as the comments also reflect, I was looking for minimal overhead especially wrt. the way it's currently used. It can be later improved with write buffering (if deemed necessary), and using e.g. writev/readv.

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 
That is what the if (stream->rbuflen < r) is is preventing on line 216. It could be written instead as "if (r > stream->rbuflen)" to be more clear.


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
This should not really matter, since reading socket can give a short read at any point. If this is useful probably depends on the caller's innerloop vs. iostream reading innerloop (potentially ssl code) cost. I think in practice there is not a big difference. But I'm ok to add the code.


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 ca
This functions was modeled after stdio fgets(). I'm ok to add more accessor if wanted, but is it useful to do so if there's no users for them? It'd be just dead code.

Count = 0 should not be used. Count is the size of the buffer, and thus maximum amount of bytes to read. Assuming some storage in *buf with count=0 is bound to result in programming erros. Thus count=0 should never be used.

The point of ast_iostream_read is to be the fast-path, so I'd rather special case it. It reads directly to user supplied buffers when possible. E.g. SSL layer already buffers internally, so better to avoid one memcpy if possible.

Generally all calling sites either: use this function, or ast_iostream_read with large buffers. Thus I wanted to avoid unconditional usage of buffering on iostream.c level.


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?
ast_iostream_read() drains it first. iostream_read() is the function that works on the fd/ssl object. Might be useful to rename that to something more explicit. Like ast_iostream_do_read() or ast_iostream_read_fd().


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 fun
All users so far were just calling write+flush. So I did not see the benefit for it yet. iostreams are mostly used for sockets, so flushing always makes sense. This also avoids one memcpy. However, if it's desired, adding write buffering can be implemented. But I'd prefer to see it as separate function ast_iostream_write_buffered() or adding some control flag that says whether to buffer or not. That way we can avoid memcpy's where possible.


Line 401: int ast_iostream_close(struct ast_iostream *stream)
> Would it make sense to move all the code for this function into the destruc
The existing code supported explicit close. I guess the point is the be able to shutdown()+close() the fd (to save system resources and drop e.g. evil connection) even when some other object is still holding references.


Line 483: 	stream = tcptls_stream_alloc();
> Pass 'fd' in to the alloc function and initialize is there. This function s
Seems tcptls_stream_alloc() is only called from here. I'll just delete that function and write it out here.


-- 
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-Reviewer: Timo Teräs <timo.teras at iki.fi>
Gerrit-HasComments: Yes