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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 10 Feb 2014 22:57:15 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> 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.

I understand what you're saying, and I don't want people to do needless and
endless respins, but letting bisect break at will doesn't seem a good option
either.

What other options do we have? What's the general QEMU directive in cases like
this?

 1. Do what you did in commit 27d59ccd?

 2. Apply it and let bisect break?

 3. Drop -Werror?

> 
> 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
>