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
