On 06/07/2018 09:59 PM, Eric Blake wrote:
On 06/07/2018 01:28 PM, Ari Sundholm wrote:
On 06/07/2018 07:13 PM, no-re...@patchew.org wrote:
Hi,
This series failed build test on s390x host. Please find the details
below.
/var/tmp/patchew-tester-tmp-8bz4jnox/src/block/blklogwrites.c: In
function ‘blk_log_writes_refresh_filename’:
/var/tmp/patchew-tester-tmp-8bz4jnox/src/block/blklogwrites.c:136:32:
error: ‘%s’ directive output may be truncated writing up to 4095
bytes into a region of size 4083 [-Werror=format-truncation=]
"blklogwrites:%s:%s",
^~
In file included from /usr/include/stdio.h:939:0,
from
/var/tmp/patchew-tester-tmp-8bz4jnox/src/include/qemu/osdep.h:68,
from
/var/tmp/patchew-tester-tmp-8bz4jnox/src/block/blklogwrites.c:12:
/usr/include/bits/stdio2.h:64:10: note: ‘__builtin___snprintf_chk’
output between 15 and 8205 bytes into a destination of size 4096
return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
__bos (__s), __fmt, __va_arg_pack ());
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/var/tmp/patchew-tester-tmp-8bz4jnox/src/rules.mak:69:
block/blklogwrites.o] Error 1
make: *** Waiting for unfinished jobs....
CC block/qapi.o
=== OUTPUT END ===
Test command exited with code: 2
Given that blkverify.c has a similar snprintf() call, with the
exception that it checks the return value in case the string was
truncated, am I safe in assuming that adding a check for the return
value of snprintf() fixes this one? I don't really see anything else I
could do about the error.
Well, ideally we'd g_strdup_printf() the string (and g_free() it later)
rather than use a fixed-size array, if we HAVE to produce a
legacy-format name in the first place. But if we stick with a
fixed-width buffer, perhaps checking for snprintf() truncation, and
returning NULL in that case, is enough to force a fallback to a
pseudo-JSON string so that the end user doesn't lose information and the
compiler doesn't complain about failure to check for truncation.
The question of whether we should generate the filename at all is
something I've been wondering about. If we do generate it,
bs->exact_filename having a fixed length is something we can't (easily)
do anything about. The .bdrv_refresh_filename callback returns void, so
there isn't really a way to tell the caller about the truncation if it
does happen.
I think that, for now, I will fix the code to handle this part like
blkverify does: check for truncation and make the filename an empty
string if that happened. If we want to replace this with something like
just a bs->exact_filename[0] = '\0' without even trying to construct a
filename, that is something that can be done in a later version if we're
unsure now.
Thanks,
Ari Sundholm
a...@tuxera.com
