[PATCH] src/serial.c: Add serial helper read and write function

[Peter Stuge]

Alvaro Neira wrote:
> +++ b/src/serial.c
.
> +int
> +osmo_serial_read(int fd, struct msgb *msg, int numbytes)
> +{
> +	int rc, bread = 0;

This is a nice place to add a bounds check..


> +	while (bread < numbytes) {
> +		rc = read(fd, msg->tail, numbytes - bread);

..so that there is guaranteed room in the msgb for the requested
number of bytes.


> +		if (rc < 0) {
> +			msgb_free(msg);
> +			return -1;
> +		}

Are you sure that these functions should destroy the msgb on errors?

Also: What about the bytes which have already been read, in case of
error? The proposed code discards those bytes. Is that OK? (I don't
know the answer to this; that's why I'm asking.)


> +int
> +osmo_serial_write(int fd, struct msgb *msg)
> +{
> +	int bwritten = 0, ret = 0;
> +
> +	bwritten = write(fd, msg->data, msg->len);
> +	if (bwritten < 0 || bwritten < msg->len) {

This is incorrect. Both read() and write() may return less than 'count'
bytes. You'll need to loop here too to ensure that all data has been
written to the port.


> +		msgb_free(msg);

Same question as with the _read function - should this function
destroy the msgb on errors?


> +		ret = -1;

I would like the functions to use similar flow. My personal
preference for how to handle the return value is the way you use
return in the _read function, I find that much nicer than the ret
variable in the _write function. But more importantly I think the
two should be consistent where possible.


//Peter