Re: A few WARNS6 cleanups

[Chris Pressey <cpressey@xxxxxxxxxxxxxxx>]

On Mon, 3 Jan 2005 10:54:12 +0100
"Simon 'corecode' Schubert" <corecode@xxxxxxxxxxxx> wrote:

> On 02.01.2005, at 23:13, Chris Pressey wrote:
> >> Anyways. Will be a bit more agressive in the future instead of just
> >> adding casts. The idea was to be conservative in that the reason
> >for> the original choice for the size and/or signedness of a variable
> >may> be important but not necessarily readily apparant. So addings
> >casts> felt less intrusive. In hindsight that was probably not a good
> >idea.
> > Actually, only one of them turned out to be problematic - I was
> > wrong about the loop variables in this instance, as many were
> > counting down(to below 0) as were counting up...
> >
> > You're right about the underlying cause - there are a lot of
> > variables where it doesn't make sense for them ever to be negative
> > (block size, for instance), but they're all declared as plain ints. 
> > This might be because that's how they're stored on disk - or not -
> > either way it will be non-trivial to fix this correctly.  The casts
> > are good enough for now, I think, so unless someone says otherwise,
> > I'll commit this in a day or so.
> 
> I didn't look at the patch or source, so ignore me if I'm off track.
> 
> In my opinion we should really fix issues, if there are some, and not 
> just cover them by silencing gcc via casts. If there is a problem with
> signedness, there might be as well be one when just casting. Code
> needs to be examined carefully to actually fix those problems.

But, consider where some of these problems reside.  From ufs/fs.h:

/*
 * Super block for an FFS file system.
 */
struct fs {
	...
	int32_t	 fs_size;		/* number of blocks in fs */
	int32_t	 fs_dsize;		/* number of data blocks in fs */
	int32_t	 fs_ncg;		/* number of cylinder groups */
	int32_t	 fs_bsize;		/* size of basic blocks in fs */
	int32_t	 fs_fsize;		/* size of frag blocks in fs */
	int32_t	 fs_frag;		/* number of frags in a block in fs */
	...

None of these can ever be below zero, but they all must be signed for
the sake of compatibility.

So if I keep the variable 'fssize' in mkfs.c a signed int, there is no
warning when comparing it to the fs_size member of the fs structure, but
there is a warning when comparing it to 'memleft', which is an unsigned
long (for presumably good reason.)

On the other hand, if I make 'fssize' an unsigned int, the warning when
comparing it to 'memleft' disappears, but a warning now occurs when it
is compared with fs.fs_size.

Or to take another example, this one from SUSv3:

"SYNOPSIS

	#include <stdio.h>
	...
	int snprintf(char *restrict s, size_t n,
 	      const char *restrict format, ...);
	...

RETURN VALUE

	...
	Upon successful completion, the snprintf() function shall return the
	number of bytes that would be written to s had n been sufficiently
	large excluding the terminating null byte."

In other words, snprintf can never return a negative number.  Yet its
return value is signed.  This means that the fairly common idiom

	if (snprintf(buf, sizeof(f), f, ...) > sizeof(f)) { err(); }

will always produce a 'comparing signed with unsigned' warning.

How can we possibly "really fix" these issues, given that one resides in
the deepest guts of UFS and the other comes from on high from POSIX?

In short, IMO casts are inevitable.  And they aren't as much of a
'cover-up' as having no WARNS level at all.

-Chris