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

[Alvaro Neira Ayuso]

On Wed, Feb 12, 2014 at 07:01:19PM +0100, Peter Stuge wrote:
> Alvaro Neira wrote:
> > +++ b/src/serial.c
> ..
> > +/*! \brief Read datas from Serial interface
> 
> Minor language comment: "datum" is singular, "data" is plural
> 
> 
> > +int
> > +osmo_serial_read(int fd, struct msgb *msg, int numbytes)
> > +{
> > +	int rc, bread = 0;
> > +
> > +	if (numbytes > msg->data_len)
> > +		return -errno;
> 
> This is a pretty serious bug. What is the value of errno here?

Yes, errno don't return anything here. I'm going to change that with
-ENOSPC

> 
> 
> > +	while (bread < numbytes) {
> > +		rc = read(fd, msg->tail, numbytes - bread);
> > +		if (rc < 0)
> > +			return -errno;
> 
> 
> Please confirm that any already-received bytes should indeed be
> discarded by this function if an error occurs? It may be useful
> to document that data loss is intentional in that case.

We don't lose any data that we have readed. In all the iteration, with 
the function msg_put we add inside of msg the interval of data that we 
have readed and we update the msg->len.

> 
> And what about when read() returns 0, indicating end of file?
> (See: man 2 read)
> 
> Oh, and one more thing: Shouldn't msg->len be updated?
> 
> 
> > +/*! \brief Write datas to Serial interface
> 
> Same minor comment on "data"
> 
> 
> > +int
> > +osmo_serial_write(int fd, struct msgb *msg)
> > +{
> > +	int rc, bwritten = 0;
> > +
> > +	while (msg->len > 0) {
> > +		rc = write(fd, msg->data, msg->len);
> > +		if (rc == -1)
> > +			return -errno;
> 
> Another small thing but I think it would be nice if the code is as
> consistent as possible between the _read and _write functions, so
> rather than _read checking rc < 0 and _write checking rc == -1 have
> the code check for errors exactly the same way in both places.

Yes, i'm going to fix that.

Thanks for all Peter. I'm going to send a new version.