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

[Bart Van Assche]

On 2019-10-22 09:07, Martin Wilck wrote:
> 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?

Hi Martin,

Have you considered to use asprintf() instead of snprintf() and a check
whether the output buffer overflows? I think the former is a more
elegant solution.

Thanks,

Bart.