[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:

(4 comments)

Also while not strictly required a/some unit test(s) would be helpful and advantageous.

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);
             : 		}
> That is what the if (stream->rbuflen < r) is is preventing on line 216. It 
Yup you are correct. I had read that backwards.


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;
             : 	}
> This should not really matter, since reading socket can give a short read a
I agree it probably doesn't matter all that much. You can leave it as is if you like unless someone else has a stronger opinion.


Line 230: ssize_t ast_iostream_gets(struct ast_iostream *stream, char *buf, size_t count)
> This functions was modeled after stdio fgets(). I'm ok to add more accessor
Yes, don't add more accessors at this time. Only what is needed/used.

Could you reverse the 'gets' code then to always write to the user's buffer? Meaning write up to 'n' bytes, if a delimiter is given, searched for, and found then memcpy to rbuf everything after.

This should allow a single generic function and possibly drop a memcpy when using a delimiter (case of delimiter not found for instance). Thoughts?


Line 288: ssize_t ast_iostream_write(struct ast_iostream *stream, const void *buf, size_t size)
> All users so far were just calling write+flush. So I did not see the benefi
Makes sense. It can be added later if that particular functionality is deemed needed.


-- 
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