Re: [Qemu-devel] [PATCH v8 03/13] dump: add API to write header of flatten format

[Laszlo Ersek <lersek@redhat.com>]

On 02/10/14 20:35, Luiz Capitulino wrote:
> On Tue, 28 Jan 2014 14:21:56 +0800
> qiaonuohan <qiaonuohan@cn.fujitsu.com> wrote:
> 
>> flatten format will be used when writing kdump-compressed format. The format is
>> also used by makedumpfile, you can refer to the following URL to get more
>> detailed information about flatten format of kdump-compressed format:
>> http://sourceforge.net/projects/makedumpfile/
>>
>> The two functions here are used to write start flat header and end flat header
>> to vmcore, and they will be called later when flatten format is used.
>>
>> struct MakedumpfileHeader stored at the head of vmcore is used to indicate the
>> vmcore is in flatten format.
>>
>> struct MakedumpfileHeader {
>>     char signature[16];     /* = "makedumpfile" */
>>     int64_t type;           /* = 1 */
>>     int64_t version;        /* = 1 */
>> };
>>
>> And struct MakedumpfileDataHeader, with offset and buf_size set to -1, is used
>> to indicate the end of vmcore in flatten format.
>>
>> struct MakedumpfileDataHeader {
>>     int64_t offset;         /* = -1 */
>>     int64_t buf_size;       /* = -1 */
>> };
>>
>> Signed-off-by: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> This patch breaks git bisect:
> 
> /home/lcapitulino/work/src/upstream/qmp-unstable/dump.c:689:12: error: ‘write_start_flat_header’ defined but not used [-Werror=unused-function]
> /home/lcapitulino/work/src/upstream/qmp-unstable/dump.c:715:12: error: ‘write_end_flat_header’ defined but not used [-Werror=unused-function]
> cc1: all warnings being treated as errors
> make[1]: *** [dump.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [subdir-x86_64-softmmu] Error 2

This is one of the most useless warnings (when turned into an error).
It's common to add the building blocks first during a series, and then
to put them to use all at once, in one of the later patches. I have
faced this before (see commit 27d59ccd), and it sucks very hard.
Catching unused functions is helpful when you don't know about them. The
point of a patch series is that in patch N you know what you're going to
do in patch N+2. IOW, you know the future, while gcc doesn't.

Plus, bisection is *already* unusable with -Werror enabled. Suppose you
want to bisect a really long range, including commits that had been
written when gcc was only at, say, 4.4. At that time, gcc-4.4's -Werror
flag let some things pass that will now most certainly break your
bisection, when you compile the tree at those earlier commits with
gcc-4.8. I'm not speaking theoretically, I have factually witnessed this.

In effect, the -Werror flag *binds* a specific qemu commit to the gcc
version that is shipped by the main distros at the time of the commit.
In order to preserve all information, the commit would have to save the
gcc version to build it with, and git bisect should restore that
information (ie. require that you have that compiler version installed).

In short, -Werror and bisection don't mix. They already don't, and we
shouldn't expect them to.

In practice, in order to silence this warning, Qiao would have to move
these function definitions into the patch that uses them (bad from a
design and review standpoint), or add useless calls inside this patch
(which I did in commit 27d59ccd, and which is ridiculous).

Qiao could also play with the diagnostics pragma (ie. suppress the
warning just in this patch, just for these functions), but that pragma
only works with gcc-4.6+, if memory serves.

Thanks
Laszlo