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