Re: [Qemu-devel] [PATCH v3 00/10] New block driver: blklogwrites

[Ari Sundholm]

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, address@hidden 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
address@hidden