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