[dm-devel] [PATCH 45/72] libmultipath: fix -Wsign-compare warnings with snprintf()

[Martin Wilck]

Hello Bart,

On Sat, 2019-10-12 at 15:59 -0700, Bart Van Assche wrote:
> On 2019-10-12 14:28, Martin Wilck wrote:
> > -	if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) >=
> > bufsiz)
> > +	if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part)
> > +	    >= (int)bufsiz)
> >  		return 0;
> 
> Please don't insert casts like this. I think enabling -Wsign-compare
> is
> wrong because it makes the source code ugly.

The casts make it explicit which signedness is intended, which is a
good thing IMO, better than the compiler trying to figure it out
using implicit type conversion. Enabling the warning will help avoid
subtle programming errors in the future, by forcing us to think twice
about signedness. Considering that, isn't this ugliness - which I don't
dispute - a relatively minor issue?

In this particular case, we're dealing with a historically-caused
property of snprintf(), as you are probably aware 
(https://stackoverflow.com/questions/45740276/why-does-printf-return-an-int-instead-of-a-size-t-in-c),
so I'd argue the ugliness is forced upon us, sort of.

We can hide the ugliness in a macro if you prefer. Actually, we have
safe_snprintf() already. We just need to use it in all places where
this kind of comparison is made. Would that be acceptable to you?

Thanks,
Martin