Re: Type mismatches in safe_read and friends?

From: Shawn O. Pearce <spearce@spearce.org>
Date: Mar 26, 2007, 16:47 UTC
Message-ID: <20070326164755.GH13247@spearce.org>
In-Reply-To: <4607D4F6.80703@dawes.za.net>

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.

...

Show 6 quoted lines

> 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.