Type mismatches in safe_read and friends?

Type mismatches in safe_read and friends?

[Rogan Dawes]

Hi folks,

I'm starting to learn a little C, and I figured I'd learn from the 
masters ;-) I needed to read in some data from the network, and I 
figured the safe_* calls would be a good example of how to do it correctly.

So, I took a look, and found:

static void safe_read(int fd, void *buffer, unsigned size)
{
         int n = 0;

         while (n < size) {
                 int ret = xread(fd, (char *) buffer + n, size - n);
                 if (ret < 0)
                         die("read error (%s)", strerror(errno));
                 if (!ret)
                         die("The remote end hung up unexpectedly");
                 n += ret;
         }
}


Surely size and 'n' should have the same signed-ness?

And, in fact, shouldn't they actually be size_t, rather than 'int', 
since xread is defined as:

static inline ssize_t xread(int fd, void *buf, size_t len)
{
         ssize_t nr;
         while (1) {
                 nr = read(fd, buf, len);
                 if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
                         continue;
                 return nr;
         }
}

And finally, 'ret' in safe_read should be a 'ssize_t', not an int, right?

Or is it just a case that we don't really care, since we control the 
ranges of the values, and the underlying types are int anyway? Patches 
to follow if I get an indication that anyone cares, otherwise I'd be 
posting my question to a C newbies group. ;-)

Rogan

Re: Type mismatches in safe_read and friends?

[Shawn O. Pearce]

Rogan Dawes <lists@dawes.za.net> wrote:
> I'm starting to learn a little C, and I figured I'd learn from the 
> masters ;-) I needed to read in some data from the network, and I 
> figured the safe_* calls would be a good example of how to do it correctly.
...
> static void safe_read(int fd, void *buffer, unsigned size)
> {
>         int n = 0;
> 
>         while (n < size) {
>                 int ret = xread(fd, (char *) buffer + n, size - n);
...
> Surely size and 'n' should have the same signed-ness?

Gah.  Yes.  And ret should be ssize_t.
 
> And, in fact, shouldn't they actually be size_t, rather than 'int', 
> since xread is defined as:

Yes.

> static inline ssize_t xread(int fd, void *buf, size_t len)
...
> And finally, 'ret' in safe_read should be a 'ssize_t', not an int, right?

Oh, I see you noticed that too.  ;-)
 
> Or is it just a case that we don't really care, since we control the 
> ranges of the values, and the underlying types are int anyway? Patches 
> to follow if I get an indication that anyone cares, otherwise I'd be 
> posting my question to a C newbies group. ;-)

It is sort of a case we don't care.  These probably should be fixed.
A patch would be nice.  You want to learn C...  ;-)

We currently assume that sizeof(unsigned) == sizeof(int) == 4,
and that nobody is crazy enough to call this functions with values
over ~2,000,000,000 so we don't practically have signed/unsigned
issues here.  Right now anyway.  But it shouldn't be like this.

So size_t/ssize_t are the right types.


The one that cracks me up is what moron declared read(2) to take
size_t as the input argument and ssize_t as the return value.
So I can pass in a value that if successfully read by the kernel
will actually be < 0 upon return, making my code think the read
call failed - but it didn't.  Riiiiiiiiight.

xread was just following that standard, broken model.

-- 
Shawn.

Re: Type mismatches in safe_read and friends?

[Rogan Dawes]

Shawn O. Pearce wrote:
>  
>> Or is it just a case that we don't really care, since we control the 
>> ranges of the values, and the underlying types are int anyway? Patches 
>> to follow if I get an indication that anyone cares, otherwise I'd be 
>> posting my question to a C newbies group. ;-)
> 
> It is sort of a case we don't care.  These probably should be fixed.
> A patch would be nice.  You want to learn C...  ;-)
> 

Ok. So I did a little patch, but I'm not sure whether I solved anything. 
Now we have an implicit cast from size_t to ssize_t in packet_read_line.

I guess this echoes your comment about requesting a large size_t, and 
getting a ssize_t result back. I suppose in theory we should be refusing 
to handle a length greater than that which would fit into a ssize_t? Or 
simply making sure to always return data smaller than max(ssize_t)?

Patch to follow.

Rogan