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. -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
