Re: [Qemu-devel] [PATCH v8 03/13] dump: add API to write header of flatten format
Laszlo Ersek ler...@redhat.com writes:
On 02/10/14 23:48, Luiz Capitulino wrote:
On Mon, 10 Feb 2014 22:57:15 +0100
Laszlo Ersek ler...@redhat.com wrote:
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 commit 27d59ccd, I introduced the create_blob_file() function, in
tests/i440fx-test.c. The patch was really about nothing else than
introducing this function (which would have made no sense outside of
said C file, so I made it static).
(The new function was to be utilized in the next patch (3bcc77ae).)
Of course gcc whined at 27d59ccd, and I was forced to call
create_blob_file() from main() just to shut it up. This function call
made absolutely no sense. I only added the call in order to convince gcc
that create_blob_file() was not some abandoned, useless function.
Of course, when the tree is built at 27d59ccd, *and* tests/i440fx-test
is run, then create_blob_file() runs too, and it *has* an effect. A blob
file is indeed created and unlinked. In other words, in that commit the
utility function is actually exercised, *outside* its intended
scope/context, just to shut up gcc.
To follow suit, Qiao would have to call write_start_flat_header() and
write_end_flat_header() *somewhere* in patch v8 03/13. It would be a
terrible thing to do. It makes absolutely no sense to call these
functions in this patch. And I don't think we could trick gcc with an
if (0) {...}, because the optimizer would eliminate the call and we'd
be left with the original warning/error.
Summary: in commit 27d59ccd, I did a horrible hack to shut up gcc. It
was only permissible because the hack affected just a test case. It
didn't affect production code that you actually might want to bisect.
Ugh!
The general qemu (and edk2) approach is to rape the code until the
*contemporary* gcc / compiler of choice shuts up. Then, at *real* bisect
time later down the road, with a new compiler release, act surprised
when the tree doesn't build. Then repeat/restart the bisection with
-Werror disabled.
I always configure --disable-werror. It doesn't make the sky fall.
My specific proposal is to test-build the tree at all patches in the
series, *except* at the last one, with -Werror disabled. Then build the
tree at the final patch with -Werror enabled.
My proposal is a generalization of yours: use a bit of common sense for
a change :)
Re: [Qemu-devel] [PATCH v8 03/13] dump: add API to write header of flatten format
On Tue, 28 Jan 2014 14:21:56 +0800
qiaonuohan qiaonuo...@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 qiaonuo...@cn.fujitsu.com
Reviewed-by: Laszlo Ersek ler...@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
---
dump.c| 42 ++
include/sysemu/dump.h | 17 +
2 files changed, 59 insertions(+), 0 deletions(-)
diff --git a/dump.c b/dump.c
index c9d3492..f233b3e 100644
--- a/dump.c
+++ b/dump.c
@@ -686,6 +686,48 @@ static int create_vmcore(DumpState *s)
return 0;
}
+static int write_start_flat_header(int fd)
+{
+uint8_t *buf;
+MakedumpfileHeader mh;
+int ret = 0;
+
+memset(mh, 0, sizeof(mh));
+strncpy(mh.signature, MAKEDUMPFILE_SIGNATURE,
+strlen(MAKEDUMPFILE_SIGNATURE));
+
+mh.type = cpu_to_be64(TYPE_FLAT_HEADER);
+mh.version = cpu_to_be64(VERSION_FLAT_HEADER);
+
+buf = g_malloc0(MAX_SIZE_MDF_HEADER);
+memcpy(buf, mh, sizeof(mh));
+
+size_t written_size;
+written_size = qemu_write_full(fd, buf, MAX_SIZE_MDF_HEADER);
+if (written_size != MAX_SIZE_MDF_HEADER) {
+ret = -1;
+}
+
+g_free(buf);
+return ret;
+}
+
+static int write_end_flat_header(int fd)
+{
+MakedumpfileDataHeader mdh;
+
+mdh.offset = END_FLAG_FLAT_HEADER;
+mdh.buf_size = END_FLAG_FLAT_HEADER;
+
+size_t written_size;
+written_size = qemu_write_full(fd, mdh, sizeof(mdh));
+if (written_size != sizeof(mdh)) {
+return -1;
+}
+
+return 0;
+}
+
static ram_addr_t get_start_block(DumpState *s)
{
GuestPhysBlock *block;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 19fafb2..b32b390 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -14,12 +14,29 @@
#ifndef DUMP_H
#define DUMP_H
+#define MAKEDUMPFILE_SIGNATURE makedumpfile
+#define MAX_SIZE_MDF_HEADER (4096) /* max size of
makedumpfile_header */
+#define TYPE_FLAT_HEADER(1)/* type of flattened format */
+#define VERSION_FLAT_HEADER (1)/* version of flattened format */
+#define END_FLAG_FLAT_HEADER(-1)
+
typedef struct ArchDumpInfo {
int d_machine; /* Architecture */
int d_endian; /* ELFDATA2LSB or ELFDATA2MSB */
int d_class;/* ELFCLASS32 or ELFCLASS64 */
} ArchDumpInfo;
+typedef struct QEMU_PACKED MakedumpfileHeader {
+char signature[16]; /* = makedumpfile */
+int64_t type;
+int64_t version;
+} MakedumpfileHeader;
+
+typedef struct QEMU_PACKED MakedumpfileDataHeader {
+int64_t offset;
+int64_t buf_size;
+} MakedumpfileDataHeader;
+
struct GuestPhysBlockList; /* memory_mapping.h */
int cpu_get_dump_info(ArchDumpInfo *info,
const struct GuestPhysBlockList *guest_phys_blocks);
Re: [Qemu-devel] [PATCH v8 03/13] dump: add API to write header of flatten format
On 02/10/14 20:35, Luiz Capitulino wrote:
On Tue, 28 Jan 2014 14:21:56 +0800
qiaonuohan qiaonuo...@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 qiaonuo...@cn.fujitsu.com
Reviewed-by: Laszlo Ersek ler...@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
Re: [Qemu-devel] [PATCH v8 03/13] dump: add API to write header of flatten format
On Mon, 10 Feb 2014 22:57:15 +0100
Laszlo Ersek ler...@redhat.com wrote:
On 02/10/14 20:35, Luiz Capitulino wrote:
On Tue, 28 Jan 2014 14:21:56 +0800
qiaonuohan qiaonuo...@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 qiaonuo...@cn.fujitsu.com
Reviewed-by: Laszlo Ersek ler...@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
Re: [Qemu-devel] [PATCH v8 03/13] dump: add API to write header of flatten format
On 02/10/14 23:48, Luiz Capitulino wrote:
On Mon, 10 Feb 2014 22:57:15 +0100
Laszlo Ersek ler...@redhat.com wrote:
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 commit 27d59ccd, I introduced the create_blob_file() function, in
tests/i440fx-test.c. The patch was really about nothing else than
introducing this function (which would have made no sense outside of
said C file, so I made it static).
(The new function was to be utilized in the next patch (3bcc77ae).)
Of course gcc whined at 27d59ccd, and I was forced to call
create_blob_file() from main() just to shut it up. This function call
made absolutely no sense. I only added the call in order to convince gcc
that create_blob_file() was not some abandoned, useless function.
Of course, when the tree is built at 27d59ccd, *and* tests/i440fx-test
is run, then create_blob_file() runs too, and it *has* an effect. A blob
file is indeed created and unlinked. In other words, in that commit the
utility function is actually exercised, *outside* its intended
scope/context, just to shut up gcc.
To follow suit, Qiao would have to call write_start_flat_header() and
write_end_flat_header() *somewhere* in patch v8 03/13. It would be a
terrible thing to do. It makes absolutely no sense to call these
functions in this patch. And I don't think we could trick gcc with an
if (0) {...}, because the optimizer would eliminate the call and we'd
be left with the original warning/error.
Summary: in commit 27d59ccd, I did a horrible hack to shut up gcc. It
was only permissible because the hack affected just a test case. It
didn't affect production code that you actually might want to bisect.
The general qemu (and edk2) approach is to rape the code until the
*contemporary* gcc / compiler of choice shuts up. Then, at *real* bisect
time later down the road, with a new compiler release, act surprised
when the tree doesn't build. Then repeat/restart the bisection with
-Werror disabled.
My specific proposal is to test-build the tree at all patches in the
series, *except* at the last one, with -Werror disabled. Then build the
tree at the final patch with -Werror enabled.
Thanks!
Laszlo
[Qemu-devel] [PATCH v8 03/13] dump: add API to write header of flatten format
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 qiaonuo...@cn.fujitsu.com
Reviewed-by: Laszlo Ersek ler...@redhat.com
---
dump.c| 42 ++
include/sysemu/dump.h | 17 +
2 files changed, 59 insertions(+), 0 deletions(-)
diff --git a/dump.c b/dump.c
index c9d3492..f233b3e 100644
--- a/dump.c
+++ b/dump.c
@@ -686,6 +686,48 @@ static int create_vmcore(DumpState *s)
return 0;
}
+static int write_start_flat_header(int fd)
+{
+uint8_t *buf;
+MakedumpfileHeader mh;
+int ret = 0;
+
+memset(mh, 0, sizeof(mh));
+strncpy(mh.signature, MAKEDUMPFILE_SIGNATURE,
+strlen(MAKEDUMPFILE_SIGNATURE));
+
+mh.type = cpu_to_be64(TYPE_FLAT_HEADER);
+mh.version = cpu_to_be64(VERSION_FLAT_HEADER);
+
+buf = g_malloc0(MAX_SIZE_MDF_HEADER);
+memcpy(buf, mh, sizeof(mh));
+
+size_t written_size;
+written_size = qemu_write_full(fd, buf, MAX_SIZE_MDF_HEADER);
+if (written_size != MAX_SIZE_MDF_HEADER) {
+ret = -1;
+}
+
+g_free(buf);
+return ret;
+}
+
+static int write_end_flat_header(int fd)
+{
+MakedumpfileDataHeader mdh;
+
+mdh.offset = END_FLAG_FLAT_HEADER;
+mdh.buf_size = END_FLAG_FLAT_HEADER;
+
+size_t written_size;
+written_size = qemu_write_full(fd, mdh, sizeof(mdh));
+if (written_size != sizeof(mdh)) {
+return -1;
+}
+
+return 0;
+}
+
static ram_addr_t get_start_block(DumpState *s)
{
GuestPhysBlock *block;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 19fafb2..b32b390 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -14,12 +14,29 @@
#ifndef DUMP_H
#define DUMP_H
+#define MAKEDUMPFILE_SIGNATURE makedumpfile
+#define MAX_SIZE_MDF_HEADER (4096) /* max size of makedumpfile_header
*/
+#define TYPE_FLAT_HEADER(1)/* type of flattened format */
+#define VERSION_FLAT_HEADER (1)/* version of flattened format */
+#define END_FLAG_FLAT_HEADER(-1)
+
typedef struct ArchDumpInfo {
int d_machine; /* Architecture */
int d_endian; /* ELFDATA2LSB or ELFDATA2MSB */
int d_class;/* ELFCLASS32 or ELFCLASS64 */
} ArchDumpInfo;
+typedef struct QEMU_PACKED MakedumpfileHeader {
+char signature[16]; /* = makedumpfile */
+int64_t type;
+int64_t version;
+} MakedumpfileHeader;
+
+typedef struct QEMU_PACKED MakedumpfileDataHeader {
+int64_t offset;
+int64_t buf_size;
+} MakedumpfileDataHeader;
+
struct GuestPhysBlockList; /* memory_mapping.h */
int cpu_get_dump_info(ArchDumpInfo *info,
const struct GuestPhysBlockList *guest_phys_blocks);
--
1.7.1
