[PATCH 0/4] Replace lseek..write/read to pwrite/pread

[PATCH 0/4] Replace lseek..write/read to pwrite/pread

[Wang Nan]

In original code there are many operations read from /write to specific
positions of a file. This series of patches replace such patterns to
pread/pwrite calls, reduces more than 100 lines of code.

Wang Nan (4):
  makedumpfile: redefine numerical limitaction macros.
  makedumpfile: cleanup non-standard ULONGLONG_MAX macros
  makedumpfile: add -D_GNU_SOURCE to CFLAGS
  makedumpfile: use pread/pwrite to eliminate lseek

 Makefile       |   4 +-
 common.h       |  19 ++++--
 makedumpfile.c | 198 ++++++++++++---------------------------------------------
 3 files changed, 58 insertions(+), 163 deletions(-)

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Cc: Petr Tesarik <ptesarik@suse.cz>
Cc: kexec@lists.infradead.org
Cc: Geng Hui <hui.geng@huawei.com>
Cc: Liu Hua <sdu.liu@huawei.com>

-- 
1.8.4


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 1/4] makedumpfile: redefine numerical limitaction macros.

[Wang Nan]

From: Wang Nan <pi3orama@gmail.com>

According to C standard, numerical limitations macros such as ULONG_MAX
should be defined in <limits.h>, and must be defined as "constant
expressions suitable for use in #if preprocessing directives." (see
"Numerical limits" section in the standard).

Original definition in common.h breaks this rule:

 #define LONG_MAX ((long)(~0UL>>1))

which causes macros like following failure:

 #if LONG_MAX == 2147483647
 # define LONG_BIT	32
 #else
 # define LONG_BIT	64
 #endif

Unfortunately, the above code piece is taken from real glibc header
(/usr/include/bits/xopen_lim.h), which is happen to be included by
<limits.h> if _GNU_SOURCE is defined.

This patch include <limits.h> in common.h to use C standard numerical
macros. For system without such macros defined by C, this patch also
defines L(L)ONG_MAX in a standard compatible way. By checking wich

gcc -dM -E - <<<''

we know that __LONG_MAX__ and __LLONG_MAX__ macros should be defined by
gcc by default. Definition of ULONG_MAX and ULLONG_MAX are taken from
gcc standard include file (include-fixed/limits.h).

In addition, macro ULONGLONG_MAX is nonstandard, the standard way for
defining max ulonglong is ULLONG_MAX.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Cc: Petr Tesarik <ptesarik@suse.cz>
Cc: kexec@lists.infradead.org
Cc: Geng Hui <hui.geng@huawei.com>
Cc: Liu Hua <sdu.liu@huawei.com>

---
 common.h | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/common.h b/common.h
index 6ad3ca7..124f107 100644
--- a/common.h
+++ b/common.h
@@ -16,17 +16,29 @@
 #ifndef _COMMON_H
 #define _COMMON_H
 
+#include <limits.h>
+
 #define TRUE		(1)
 #define FALSE		(0)
 #define ERROR		(-1)
 
 #ifndef LONG_MAX
-#define LONG_MAX	((long)(~0UL>>1))
+# warning LONG_MAX should have been defined in <limits.h>
+# define LONG_MAX	__LONG_MAX__
 #endif
 #ifndef ULONG_MAX
-#define ULONG_MAX	(~0UL)
+# warning ULONG_MAX should have been defined in <limits.h>
+# define ULONG_MAX	(LONG_MAX * 2UL + 1UL)
+#endif
+#ifndef LLONG_MAX
+# warning LLONG_MAX should have been defined in <limits.h>
+# define LLONG_MAX __LONG_LONG_MAX__
+#endif
+#ifndef ULLONG_MAX
+# warning ULLONG_MAX should have been defined in <limits.h>
+# define ULLONG_MAX (LLONG_MAX * 2ULL + 1ULL)
 #endif
-#define ULONGLONG_MAX	(~0ULL)
+#define ULONGLONG_MAX	ULLONG_MAX
 
 #define MAX(a,b)	((a) > (b) ? (a) : (b))
 #define MIN(a,b)	((a) < (b) ? (a) : (b))
-- 
1.8.4


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 2/4] makedumpfile: cleanup non-standard ULONGLONG_MAX macros

[Wang Nan]

Macro ULONGLONG_MAX is non-standard, the standard way to defining
maximum limitation of unsigned long long is ULLONG_MAX, which is defined
in <limits.h> by C standard.

This patch replace all ULONGLONG_MAX to ULLONG_MAX.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Cc: Petr Tesarik <ptesarik@suse.cz>
Cc: kexec@lists.infradead.org
Cc: Geng Hui <hui.geng@huawei.com>
Cc: Liu Hua <sdu.liu@huawei.com>

---
 common.h       |  3 +--
 makedumpfile.c | 10 +++++-----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/common.h b/common.h
index 124f107..b86c57a 100644
--- a/common.h
+++ b/common.h
@@ -38,7 +38,6 @@
 # warning ULLONG_MAX should have been defined in <limits.h>
 # define ULLONG_MAX (LLONG_MAX * 2ULL + 1ULL)
 #endif
-#define ULONGLONG_MAX	ULLONG_MAX
 
 #define MAX(a,b)	((a) > (b) ? (a) : (b))
 #define MIN(a,b)	((a) < (b) ? (a) : (b))
@@ -52,7 +51,7 @@
  */
 #define NOT_MEMMAP_ADDR	(0x0)
 #define NOT_KV_ADDR	(0x0)
-#define NOT_PADDR	(ULONGLONG_MAX)
+#define NOT_PADDR	(ULLONG_MAX)
 
 #endif  /* COMMON_H */
 
diff --git a/makedumpfile.c b/makedumpfile.c
index ce4a866..33c378d 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -3797,7 +3797,7 @@ unsigned long long
 page_to_pfn(unsigned long page)
 {
 	unsigned int num;
-	unsigned long long pfn = ULONGLONG_MAX;
+	unsigned long long pfn = ULLONG_MAX;
 	unsigned long long index = 0;
 	struct mem_map_data *mmd;
 
@@ -3813,9 +3813,9 @@ page_to_pfn(unsigned long page)
 		pfn = mmd->pfn_start + index;
 		break;
 	}
-	if (pfn == ULONGLONG_MAX) {
+	if (pfn == ULLONG_MAX) {
 		ERRMSG("Can't convert the address of page descriptor (%lx) to pfn.\n", page);
-		return ULONGLONG_MAX;
+		return ULLONG_MAX;
 	}
 	return pfn;
 }
@@ -3852,7 +3852,7 @@ reset_bitmap_of_free_pages(unsigned long node_zones, struct cycle *cycle)
 			for (;curr != head;) {
 				curr_page = curr - OFFSET(page.lru);
 				start_pfn = page_to_pfn(curr_page);
-				if (start_pfn == ULONGLONG_MAX)
+				if (start_pfn == ULLONG_MAX)
 					return FALSE;
 
 				if (!readmem(VADDR, curr+OFFSET(list_head.prev),
@@ -4678,7 +4678,7 @@ __exclude_unnecessary_pages(unsigned long mem_map,
 	/*
 	 * Refresh the buffer of struct page, when changing mem_map.
 	 */
-	pfn_read_start = ULONGLONG_MAX;
+	pfn_read_start = ULLONG_MAX;
 	pfn_read_end   = 0;
 
 	for (pfn = pfn_start; pfn < pfn_end; pfn++, mem_map += SIZE(page)) {
-- 
1.8.4


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS

[Wang Nan]

This patch is preparation for introduce pread/pwrite.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Cc: Petr Tesarik <ptesarik@suse.cz>
Cc: kexec@lists.infradead.org
Cc: Geng Hui <hui.geng@huawei.com>
Cc: Liu Hua <sdu.liu@huawei.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 68db947..1c65e61 100644
--- a/Makefile
+++ b/Makefile
@@ -9,11 +9,11 @@ CC	= gcc
 endif
 
 CFLAGS = -g -O2 -Wall -D_FILE_OFFSET_BITS=64 \
-	  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE \
+	  -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_GNU_SOURCE \
 	  -DVERSION='"$(VERSION)"' -DRELEASE_DATE='"$(DATE)"' \
 	  -I/home/w00229757/makedumpfile/elfutils-root/include
 CFLAGS_ARCH	= -g -O2 -Wall -D_FILE_OFFSET_BITS=64 \
-		    -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE
+		    -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_GNU_SOURCE
 # LDFLAGS = -L/usr/local/lib -I/usr/local/include
 LDFLAGS = -L/home/w00229757/makedumpfile/elfutils-root/lib
 
-- 
1.8.4


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 4/4] makedumpfile: use pread/pwrite to eliminate lseek

[Wang Nan]

Original code use lseek..read/write pattern for read from/write to
specific position of a file. This patch replaces them by pread/pwrite,
reduces more than 100 LOC.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
Cc: Petr Tesarik <ptesarik@suse.cz>
Cc: kexec@lists.infradead.org
Cc: Geng Hui <hui.geng@huawei.com>
Cc: Liu Hua <sdu.liu@huawei.com>
---
 makedumpfile.c | 188 +++++++++++----------------------------------------------
 1 file changed, 36 insertions(+), 152 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 33c378d..934db88 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -25,6 +25,8 @@
 #include <sys/time.h>
 #include <limits.h>
 
+#include <unistd.h>
+
 struct symbol_table	symbol_table;
 struct size_table	size_table;
 struct offset_table	offset_table;
@@ -259,16 +261,11 @@ read_page_desc(unsigned long long paddr, page_desc_t *pd)
 	pfn = paddr_to_pfn(paddr);
 	desc_pos = pfn_to_pos(pfn);
 	offset += (off_t)desc_pos * sizeof(page_desc_t);
-	if (lseek(info->fd_memory, offset, SEEK_SET) < 0) {
-		ERRMSG("Can't seek %s. %s\n",
-				 info->name_memory, strerror(errno));
-		return FALSE;
-	}
 
 	/*
 	 * Read page descriptor
 	 */
-	if (read(info->fd_memory, pd, sizeof(*pd)) != sizeof(*pd)) {
+	if (pread(info->fd_memory, pd, sizeof(*pd), offset) != sizeof(*pd)) {
 		ERRMSG("Can't read %s. %s\n",
 				info->name_memory, strerror(errno));
 		return FALSE;
@@ -374,8 +371,6 @@ next_region:
 static int
 read_from_vmcore(off_t offset, void *bufptr, unsigned long size)
 {
-	const off_t failed = (off_t)-1;
-
 	if (info->flag_usemmap == MMAP_ENABLE &&
 	    page_is_fractional(offset) == FALSE) {
 		if (!read_with_mmap(offset, bufptr, size)) {
@@ -392,13 +387,7 @@ read_from_vmcore(off_t offset, void *bufptr, unsigned long size)
 			read_from_vmcore(offset, bufptr, size);
 		}
 	} else {
-		if (lseek(info->fd_memory, offset, SEEK_SET) == failed) {
-			ERRMSG("Can't seek the dump memory(%s). (offset: %llx) %s\n",
-			       info->name_memory, (unsigned long long)offset, strerror(errno));
-			return FALSE;
-		}
-
-		if (read(info->fd_memory, bufptr, size) != size) {
+		if (pread(info->fd_memory, bufptr, size, offset) != size) {
 			ERRMSG("Can't read the dump memory(%s). %s\n",
 			       info->name_memory, strerror(errno));
 			return FALSE;
@@ -520,18 +509,12 @@ readpage_kdump_compressed(unsigned long long paddr, void *bufptr)
 		return FALSE;
 	}
 
-	if (lseek(info->fd_memory, pd.offset, SEEK_SET) < 0) {
-		ERRMSG("Can't seek %s. %s\n",
-				info->name_memory, strerror(errno));
-		return FALSE;
-	}
-
 	/*
 	 * Read page data
 	 */
 	rdbuf = pd.flags & (DUMP_DH_COMPRESSED_ZLIB | DUMP_DH_COMPRESSED_LZO |
 		DUMP_DH_COMPRESSED_SNAPPY) ? buf : bufptr;
-	if (read(info->fd_memory, rdbuf, pd.size) != pd.size) {
+	if (pread(info->fd_memory, rdbuf, pd.size, pd.offset) != pd.size) {
 		ERRMSG("Can't read %s. %s\n",
 				info->name_memory, strerror(errno));
 		return FALSE;
@@ -1555,7 +1538,6 @@ get_str_osrelease_from_vmlinux(void)
 	struct utsname system_utsname;
 	unsigned long long utsname;
 	off_t offset;
-	const off_t failed = (off_t)-1;
 
 	/*
 	 * Get the kernel version.
@@ -1576,11 +1558,7 @@ get_str_osrelease_from_vmlinux(void)
 		    utsname);
 		return FALSE;
 	}
-	if (lseek(fd, offset, SEEK_SET) == failed) {
-		ERRMSG("Can't seek %s. %s\n", name, strerror(errno));
-		return FALSE;
-	}
-	if (read(fd, &system_utsname, sizeof system_utsname)
+	if (pread(fd, &system_utsname, sizeof(system_utsname), offset)
 	    != sizeof system_utsname) {
 		ERRMSG("Can't read %s. %s\n", name, strerror(errno));
 		return FALSE;
@@ -2124,7 +2102,6 @@ copy_vmcoreinfo(off_t offset, unsigned long size)
 {
 	int fd;
 	char buf[VMCOREINFO_BYTES];
-	const off_t failed = (off_t)-1;
 
 	if (!offset || !size)
 		return FALSE;
@@ -2134,12 +2111,7 @@ copy_vmcoreinfo(off_t offset, unsigned long size)
 		    info->name_vmcoreinfo, strerror(errno));
 		return FALSE;
 	}
-	if (lseek(info->fd_memory, offset, SEEK_SET) == failed) {
-		ERRMSG("Can't seek the dump memory(%s). %s\n",
-		    info->name_memory, strerror(errno));
-		return FALSE;
-	}
-	if (read(info->fd_memory, &buf, size) != size) {
+	if (pread(info->fd_memory, &buf, size, offset) != size) {
 		ERRMSG("Can't read the dump memory(%s). %s\n",
 		    info->name_memory, strerror(errno));
 		return FALSE;
@@ -3318,12 +3290,7 @@ set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn,
 	new_offset = bitmap->offset + BUFSIZE_BITMAP * (pfn / PFN_BUFBITMAP);
 
 	if (0 <= bitmap->no_block && old_offset != new_offset) {
-		if (lseek(bitmap->fd, old_offset, SEEK_SET) < 0 ) {
-			ERRMSG("Can't seek the bitmap(%s). %s\n",
-			    bitmap->file_name, strerror(errno));
-			return FALSE;
-		}
-		if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP)
+		if (pwrite(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP, old_offset)
 		    != BUFSIZE_BITMAP) {
 			ERRMSG("Can't write the bitmap(%s). %s\n",
 			    bitmap->file_name, strerror(errno));
@@ -3331,12 +3298,7 @@ set_bitmap(struct dump_bitmap *bitmap, unsigned long long pfn,
 		}
 	}
 	if (old_offset != new_offset) {
-		if (lseek(bitmap->fd, new_offset, SEEK_SET) < 0 ) {
-			ERRMSG("Can't seek the bitmap(%s). %s\n",
-			    bitmap->file_name, strerror(errno));
-			return FALSE;
-		}
-		if (read(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP)
+		if (pread(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP, new_offset)
 		    != BUFSIZE_BITMAP) {
 			ERRMSG("Can't read the bitmap(%s). %s\n",
 			    bitmap->file_name, strerror(errno));
@@ -3398,12 +3360,7 @@ sync_bitmap(struct dump_bitmap *bitmap)
 	if (bitmap->no_block < 0)
 		return TRUE;
 
-	if (lseek(bitmap->fd, offset, SEEK_SET) < 0 ) {
-		ERRMSG("Can't seek the bitmap(%s). %s\n",
-		    bitmap->file_name, strerror(errno));
-		return FALSE;
-	}
-	if (write(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP)
+	if (pwrite(bitmap->fd, bitmap->buf, BUFSIZE_BITMAP, offset)
 	    != BUFSIZE_BITMAP) {
 		ERRMSG("Can't write the bitmap(%s). %s\n",
 		    bitmap->file_name, strerror(errno));
@@ -3519,14 +3476,7 @@ is_in_segs(unsigned long long paddr)
 int
 read_cache(struct cache_data *cd)
 {
-	const off_t failed = (off_t)-1;
-
-	if (lseek(cd->fd, cd->offset, SEEK_SET) == failed) {
-		ERRMSG("Can't seek the dump file(%s). %s\n",
-		    cd->file_name, strerror(errno));
-		return FALSE;
-	}
-	if (read(cd->fd, cd->buf, cd->cache_size) != cd->cache_size) {
+	if (pread(cd->fd, cd->buf, cd->cache_size, cd->offset) != cd->cache_size) {
 		ERRMSG("Can't read the dump file(%s). %s\n",
 		    cd->file_name, strerror(errno));
 		return FALSE;
@@ -4892,24 +4842,16 @@ copy_bitmap(void)
 
 	offset = 0;
 	while (offset < (info->len_bitmap / 2)) {
-		if (lseek(info->bitmap1->fd, info->bitmap1->offset + offset,
-		    SEEK_SET) == failed) {
-			ERRMSG("Can't seek the bitmap(%s). %s\n",
-			    info->name_bitmap, strerror(errno));
-			return FALSE;
-		}
-		if (read(info->bitmap1->fd, buf, sizeof(buf)) != sizeof(buf)) {
+		if (pread(info->bitmap1->fd, buf, sizeof(buf),
+					info->bitmap1->offset + offset)
+				!= sizeof(buf)) {
 			ERRMSG("Can't read the dump memory(%s). %s\n",
 			    info->name_memory, strerror(errno));
 			return FALSE;
 		}
-		if (lseek(info->bitmap2->fd, info->bitmap2->offset + offset,
-		    SEEK_SET) == failed) {
-			ERRMSG("Can't seek the bitmap(%s). %s\n",
-			    info->name_bitmap, strerror(errno));
-			return FALSE;
-		}
-		if (write(info->bitmap2->fd, buf, sizeof(buf)) != sizeof(buf)) {
+		if (pwrite(info->bitmap2->fd, buf, sizeof(buf),
+					info->bitmap2->offset + offset)
+				!= sizeof(buf)) {
 			ERRMSG("Can't write the bitmap(%s). %s\n",
 		    	info->name_bitmap, strerror(errno));
 			return FALSE;
@@ -5456,12 +5398,8 @@ write_elf_header(struct cache_data *cd_header)
 		    strerror(errno));
 		goto out;
 	}
-	if (lseek(info->fd_memory, offset_note_memory, SEEK_SET) == failed) {
-		ERRMSG("Can't seek the dump memory(%s). %s\n",
-		    info->name_memory, strerror(errno));
-		goto out;
-	}
-	if (read(info->fd_memory, buf, size_note) != size_note) {
+	if (pread(info->fd_memory, buf, size_note, offset_note_memory)
+			!= size_note) {
 		ERRMSG("Can't read the dump memory(%s). %s\n",
 		    info->name_memory, strerror(errno));
 		goto out;
@@ -5570,12 +5508,8 @@ write_kdump_header(void)
 		}
 
 		if (!info->flag_sadump) {
-			if (lseek(info->fd_memory, offset_note, SEEK_SET) < 0) {
-				ERRMSG("Can't seek the dump memory(%s). %s\n",
-				       info->name_memory, strerror(errno));
-				goto out;
-			}
-			if (read(info->fd_memory, buf, size_note) != size_note) {
+			if (pread(info->fd_memory, buf, size_note, offset_note)
+					!= size_note) {
 				ERRMSG("Can't read the dump memory(%s). %s\n",
 				       info->name_memory, strerror(errno));
 				goto out;
@@ -6586,12 +6520,7 @@ copy_eraseinfo(struct cache_data *cd_eraseinfo)
 		    strerror(errno));
 		return FALSE;
 	}
-	if (lseek(info->fd_memory, offset, SEEK_SET) < 0) {
-		ERRMSG("Can't seek the dump memory(%s). %s\n",
-		    info->name_memory, strerror(errno));
-		goto out;
-	}
-	if (read(info->fd_memory, buf, size) != size) {
+	if (pread(info->fd_memory, buf, size, offset) != size) {
 		ERRMSG("Can't read the dump memory(%s). %s\n",
 		    info->name_memory, strerror(errno));
 		goto out;
@@ -7169,12 +7098,8 @@ init_xen_crash_info(void)
 		return FALSE;
 	}
 
-	if (lseek(info->fd_memory, offset_xen_crash_info, SEEK_SET) < 0) {
-		ERRMSG("Can't seek the dump memory(%s). %s\n",
-		       info->name_memory, strerror(errno));
-		return FALSE;
-	}
-	if (read(info->fd_memory, buf, size_xen_crash_info)
+	if (pread(info->fd_memory, buf, size_xen_crash_info,
+				offset_xen_crash_info)
 	    != size_xen_crash_info) {
 		ERRMSG("Can't read the dump memory(%s). %s\n",
 		       info->name_memory, strerror(errno));
@@ -8155,12 +8080,7 @@ __read_disk_dump_header(struct disk_dump_header *dh, char *filename)
 		    filename, strerror(errno));
 		return FALSE;
 	}
-	if (lseek(fd, 0x0, SEEK_SET) < 0) {
-		ERRMSG("Can't seek a file(%s). %s\n",
-		    filename, strerror(errno));
-		goto out;
-	}
-	if (read(fd, dh, sizeof(struct disk_dump_header))
+	if (pread(fd, dh, sizeof(struct disk_dump_header), 0x0)
 	    != sizeof(struct disk_dump_header)) {
 		ERRMSG("Can't read a file(%s). %s\n",
 		    filename, strerror(errno));
@@ -8204,12 +8124,7 @@ read_kdump_sub_header(struct kdump_sub_header *kh, char *filename)
 		    filename, strerror(errno));
 		return FALSE;
 	}
-	if (lseek(fd, offset, SEEK_SET) < 0) {
-		ERRMSG("Can't seek a file(%s). %s\n",
-		    filename, strerror(errno));
-		goto out;
-	}
-	if (read(fd, kh, sizeof(struct kdump_sub_header))
+	if (pread(fd, kh, sizeof(struct kdump_sub_header), offset)
 	     != sizeof(struct kdump_sub_header)) {
 		ERRMSG("Can't read a file(%s). %s\n",
 		    filename, strerror(errno));
@@ -8373,19 +8288,11 @@ copy_same_data(int src_fd, int dst_fd, off_t offset, unsigned long size)
 		ERRMSG("Can't allocate memory.\n");
 		return FALSE;
 	}
-	if (lseek(src_fd, offset, SEEK_SET) < 0) {
-		ERRMSG("Can't seek a source file. %s\n", strerror(errno));
-		goto out;
-	}
-	if (read(src_fd, buf, size) != size) {
+	if (pread(src_fd, buf, size, offset) != size) {
 		ERRMSG("Can't read a source file. %s\n", strerror(errno));
 		goto out;
 	}
-	if (lseek(dst_fd, offset, SEEK_SET) < 0) {
-		ERRMSG("Can't seek a destination file. %s\n", strerror(errno));
-		goto out;
-	}
-	if (write(dst_fd, buf, size) != size) {
+	if (pwrite(pdst_fd, buf, size, offset) != size) {
 		ERRMSG("Can't write a destination file. %s\n", strerror(errno));
 		goto out;
 	}
@@ -8412,12 +8319,7 @@ reassemble_kdump_header(void)
 	if (!read_disk_dump_header(&dh, SPLITTING_DUMPFILE(0)))
 		return FALSE;
 
-	if (lseek(info->fd_dumpfile, 0x0, SEEK_SET) < 0) {
-		ERRMSG("Can't seek a file(%s). %s\n",
-		    info->name_dumpfile, strerror(errno));
-		return FALSE;
-	}
-	if (write(info->fd_dumpfile, &dh, sizeof(dh)) != sizeof(dh)) {
+	if (pwrite(info->fd_dumpfile, &dh, sizeof(dh), 0x0) != sizeof(dh)) {
 		ERRMSG("Can't write a file(%s). %s\n",
 		    info->name_dumpfile, strerror(errno));
 		return FALSE;
@@ -8435,12 +8337,8 @@ reassemble_kdump_header(void)
 	kh.start_pfn_64 = 0;
 	kh.end_pfn_64 = 0;
 
-	if (lseek(info->fd_dumpfile, info->page_size, SEEK_SET) < 0) {
-		ERRMSG("Can't seek a file(%s). %s\n",
-		    info->name_dumpfile, strerror(errno));
-		return FALSE;
-	}
-	if (write(info->fd_dumpfile, &kh, sizeof(kh)) != sizeof(kh)) {
+	if (pwrite(info->fd_dumpfile, &kh, sizeof(kh), info->page_size)
+			!= sizeof(kh)) {
 		ERRMSG("Can't write a file(%s). %s\n",
 		    info->name_dumpfile, strerror(errno));
 		return FALSE;
@@ -8623,22 +8521,12 @@ reassemble_kdump_pages(void)
 
 			print_progress(PROGRESS_COPY, num_dumped, num_dumpable);
 
-			if (lseek(fd, offset_ph_org, SEEK_SET) < 0) {
-				ERRMSG("Can't seek a file(%s). %s\n",
-				    SPLITTING_DUMPFILE(i), strerror(errno));
-				goto out;
-			}
-			if (read(fd, &pd, sizeof(pd)) != sizeof(pd)) {
+			if (pread(fd, &pd, sizeof(pd)) != sizeof(pd), offset_ph_org) {
 				ERRMSG("Can't read a file(%s). %s\n",
 				    SPLITTING_DUMPFILE(i), strerror(errno));
 				goto out;
 			}
-			if (lseek(fd, pd.offset, SEEK_SET) < 0) {
-				ERRMSG("Can't seek a file(%s). %s\n",
-				    SPLITTING_DUMPFILE(i), strerror(errno));
-				goto out;
-			}
-			if (read(fd, data, pd.size) != pd.size) {
+			if (pread(fd, data, pd.size, pd.offset) != pd.size) {
 				ERRMSG("Can't read a file(%s). %s\n",
 				    SPLITTING_DUMPFILE(i), strerror(errno));
 				goto out;
@@ -8692,13 +8580,9 @@ reassemble_kdump_pages(void)
 			    SPLITTING_DUMPFILE(i), strerror(errno));
 			goto out;
 		}
-		if (lseek(fd, SPLITTING_OFFSET_EI(i), SEEK_SET) < 0) {
-			ERRMSG("Can't seek a file(%s). %s\n",
-			    SPLITTING_DUMPFILE(i), strerror(errno));
-			goto out;
-		}
-		if (read(fd, data, SPLITTING_SIZE_EI(i)) !=
-						SPLITTING_SIZE_EI(i)) {
+		if (pread(fd, data, SPLITTING_SIZE_EI(i),
+					SPLITTING_OFFSET_EI(i))
+				!= SPLITTING_SIZE_EI(i)) {
 			ERRMSG("Can't read a file(%s). %s\n",
 			    SPLITTING_DUMPFILE(i), strerror(errno));
 			goto out;
-- 
1.8.4


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/4] makedumpfile: redefine numerical limitaction macros.

[Petr Tesarik]

On Sat, 26 Apr 2014 12:07:06 +0800
Wang Nan <wangnan0@huawei.com> wrote:

> From: Wang Nan <pi3orama@gmail.com>
> 
> According to C standard, numerical limitations macros such as ULONG_MAX
> should be defined in <limits.h>, and must be defined as "constant
> expressions suitable for use in #if preprocessing directives." (see
> "Numerical limits" section in the standard).
> 
> Original definition in common.h breaks this rule:
> 
>  #define LONG_MAX ((long)(~0UL>>1))
> 
> which causes macros like following failure:
> 
>  #if LONG_MAX == 2147483647
>  # define LONG_BIT	32
>  #else
>  # define LONG_BIT	64
>  #endif
> 
> Unfortunately, the above code piece is taken from real glibc header
> (/usr/include/bits/xopen_lim.h), which is happen to be included by
> <limits.h> if _GNU_SOURCE is defined.
> 
> This patch include <limits.h> in common.h to use C standard numerical
> macros. For system without such macros defined by C, this patch also
> defines L(L)ONG_MAX in a standard compatible way. By checking wich
> 
> gcc -dM -E - <<<''
> 
> we know that __LONG_MAX__ and __LLONG_MAX__ macros should be defined by
> gcc by default. Definition of ULONG_MAX and ULLONG_MAX are taken from
> gcc standard include file (include-fixed/limits.h).
> 
> In addition, macro ULONGLONG_MAX is nonstandard, the standard way for
> defining max ulonglong is ULLONG_MAX.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
> Cc: Petr Tesarik <ptesarik@suse.cz>
> Cc: kexec@lists.infradead.org
> Cc: Geng Hui <hui.geng@huawei.com>
> Cc: Liu Hua <sdu.liu@huawei.com>
> 
> ---
>  common.h | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/common.h b/common.h
> index 6ad3ca7..124f107 100644
> --- a/common.h
> +++ b/common.h
> @@ -16,17 +16,29 @@
>  #ifndef _COMMON_H
>  #define _COMMON_H
>  
> +#include <limits.h>
> +
>  #define TRUE		(1)
>  #define FALSE		(0)
>  #define ERROR		(-1)
>  
>  #ifndef LONG_MAX
> -#define LONG_MAX	((long)(~0UL>>1))
> +# warning LONG_MAX should have been defined in <limits.h>
> +# define LONG_MAX	__LONG_MAX__
>  #endif
>  #ifndef ULONG_MAX
> -#define ULONG_MAX	(~0UL)
> +# warning ULONG_MAX should have been defined in <limits.h>
> +# define ULONG_MAX	(LONG_MAX * 2UL + 1UL)
> +#endif
> +#ifndef LLONG_MAX
> +# warning LLONG_MAX should have been defined in <limits.h>
> +# define LLONG_MAX __LONG_LONG_MAX__
> +#endif
> +#ifndef ULLONG_MAX
> +# warning ULLONG_MAX should have been defined in <limits.h>
> +# define ULLONG_MAX (LLONG_MAX * 2ULL + 1ULL)
>  #endif
> -#define ULONGLONG_MAX	(~0ULL)
> +#define ULONGLONG_MAX	ULLONG_MAX

Hi Wang Nan,

is this actually needed on some known platform? If not, then I'd rather
remove all these #ifndef stanzas and rely on <limits.h>. I mean, if you
can't rely on standard C constants, then why should be the gcc-specific
pre-defined macros (__LONG_MAX__ et al.) available?

It's probably better to put the burden on the person doing the
port, because they should know what is appropriate for their compiler
and/or libc.

Just my opinion,
Petr T

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/4] makedumpfile: redefine numerical limitaction macros.

[Wang Nan]

On 2014/4/28 22:23, Petr Tesarik wrote:
> On Sat, 26 Apr 2014 12:07:06 +0800
> Wang Nan <wangnan0@huawei.com> wrote:
> 
>> From: Wang Nan <pi3orama@gmail.com>
>>
>> According to C standard, numerical limitations macros such as ULONG_MAX
>> should be defined in <limits.h>, and must be defined as "constant
>> expressions suitable for use in #if preprocessing directives." (see
>> "Numerical limits" section in the standard).
>>
>> Original definition in common.h breaks this rule:
>>
>>  #define LONG_MAX ((long)(~0UL>>1))
>>
>> which causes macros like following failure:
>>
>>  #if LONG_MAX == 2147483647
>>  # define LONG_BIT	32
>>  #else
>>  # define LONG_BIT	64
>>  #endif
>>
>> Unfortunately, the above code piece is taken from real glibc header
>> (/usr/include/bits/xopen_lim.h), which is happen to be included by
>> <limits.h> if _GNU_SOURCE is defined.
>>
>> This patch include <limits.h> in common.h to use C standard numerical
>> macros. For system without such macros defined by C, this patch also
>> defines L(L)ONG_MAX in a standard compatible way. By checking wich
>>
>> gcc -dM -E - <<<''
>>
>> we know that __LONG_MAX__ and __LLONG_MAX__ macros should be defined by
>> gcc by default. Definition of ULONG_MAX and ULLONG_MAX are taken from
>> gcc standard include file (include-fixed/limits.h).
>>
>> In addition, macro ULONGLONG_MAX is nonstandard, the standard way for
>> defining max ulonglong is ULLONG_MAX.
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp>
>> Cc: Petr Tesarik <ptesarik@suse.cz>
>> Cc: kexec@lists.infradead.org
>> Cc: Geng Hui <hui.geng@huawei.com>
>> Cc: Liu Hua <sdu.liu@huawei.com>
>>
>> ---
>>  common.h | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/common.h b/common.h
>> index 6ad3ca7..124f107 100644
>> --- a/common.h
>> +++ b/common.h
>> @@ -16,17 +16,29 @@
>>  #ifndef _COMMON_H
>>  #define _COMMON_H
>>  
>> +#include <limits.h>
>> +
>>  #define TRUE		(1)
>>  #define FALSE		(0)
>>  #define ERROR		(-1)
>>  
>>  #ifndef LONG_MAX
>> -#define LONG_MAX	((long)(~0UL>>1))
>> +# warning LONG_MAX should have been defined in <limits.h>
>> +# define LONG_MAX	__LONG_MAX__
>>  #endif
>>  #ifndef ULONG_MAX
>> -#define ULONG_MAX	(~0UL)
>> +# warning ULONG_MAX should have been defined in <limits.h>
>> +# define ULONG_MAX	(LONG_MAX * 2UL + 1UL)
>> +#endif
>> +#ifndef LLONG_MAX
>> +# warning LLONG_MAX should have been defined in <limits.h>
>> +# define LLONG_MAX __LONG_LONG_MAX__
>> +#endif
>> +#ifndef ULLONG_MAX
>> +# warning ULLONG_MAX should have been defined in <limits.h>
>> +# define ULLONG_MAX (LLONG_MAX * 2ULL + 1ULL)
>>  #endif
>> -#define ULONGLONG_MAX	(~0ULL)
>> +#define ULONGLONG_MAX	ULLONG_MAX
> 
> Hi Wang Nan,
> 
> is this actually needed on some known platform? If not, then I'd rather
> remove all these #ifndef stanzas and rely on <limits.h>. I mean, if you
> can't rely on standard C constants, then why should be the gcc-specific
> pre-defined macros (__LONG_MAX__ et al.) available?
> 

These macros exist at the first version (at makedumpfile.h), an enforced by
commit ab9c60bf (just because they conflict with limits.h ...). But I don't
think there exists a real platform without <limits.h>.

I agree with you that totally removing these macros should be better.

> It's probably better to put the burden on the person doing the
> port, because they should know what is appropriate for their compiler
> and/or libc.
> 
> Just my opinion,
> Petr T
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread

[HATAYAMA Daisuke]

From: Wang Nan <wangnan0@huawei.com>
Subject: [PATCH 0/4] Replace lseek..write/read to pwrite/pread
Date: Sat, 26 Apr 2014 12:07:05 +0800

> In original code there are many operations read from /write to specific
> positions of a file. This series of patches replace such patterns to
> pread/pwrite calls, reduces more than 100 lines of code.
> 

I'm now writing pthread support patch set and it will naturally
include pread/pwrite like this patch set.

It sounds to me that using pread/pwrite only to reduce lseek code is
weak in motivation. Is there another visible merit? For example, any
kind of performance improvement. I guess it's small even if exists
compared to I/O.

--
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread

[Petr Tesarik]

On Wed, 30 Apr 2014 20:41:38 +0900 (JST)
HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:

> From: Wang Nan <wangnan0@huawei.com>
> Subject: [PATCH 0/4] Replace lseek..write/read to pwrite/pread
> Date: Sat, 26 Apr 2014 12:07:05 +0800
> 
> > In original code there are many operations read from /write to specific
> > positions of a file. This series of patches replace such patterns to
> > pread/pwrite calls, reduces more than 100 lines of code.
> > 
> 
> I'm now writing pthread support patch set and it will naturally
> include pread/pwrite like this patch set.
> 
> It sounds to me that using pread/pwrite only to reduce lseek code is
> weak in motivation. Is there another visible merit? For example, any
> kind of performance improvement. I guess it's small even if exists
> compared to I/O.

There is no user-visible benefit just from applying the patch, that's
right.

The main benefit is that these pread/pwrite operations are atomic and do
not move the file offset, so all subprocesses (or threads) can share
the same file descriptor. This allows to remove reopen_dump_memory(),
for example.

Anyway, is improving code readability really so weak argument?

Petr T

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS

[HATAYAMA Daisuke]

From: Wang Nan <wangnan0@huawei.com>
Subject: [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS
Date: Sat, 26 Apr 2014 12:07:08 +0800

> This patch is preparation for introduce pread/pwrite.
> 

Do you explain more about _GNU_SOURCE? Did you need to define this on
your environment to build makedumpfile with pread/pwrite?

I tried to build a very simple test program using pread like

int main(void)
{
   printf("%p\n", pread);
}

on RHEL5.4, RHEL6.5 and fc20, and all were done successfully without
_GNU_SOURCE. They are all on x86_64.

I checked man pread and man pwrite on each environments for
_GNU_SOURCE but I didn't find it. What I found was _XOPEN_SOURCE
description only. For example this is man pread on RHEL6.5.

$ LANG=C man pread
PREAD(2)                   Linux Programmer's Manual                  PREAD(2)

NAME
       pread, pwrite - read from or write to a file descriptor at a given offset

SYNOPSIS
       #define _XOPEN_SOURCE 500

       #include <unistd.h>

       ssize_t pread(int fd, void *buf, size_t count, off_t offset);

       ssize_t pwrite(int fd, const void *buf, size_t count, off_t offset);

Note that just as I said the above, building was successfully done on
this environment.

--
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread

[HATAYAMA Daisuke]

From: Petr Tesarik <ptesarik@suse.cz>
Subject: Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread
Date: Wed, 30 Apr 2014 13:53:04 +0200

> On Wed, 30 Apr 2014 20:41:38 +0900 (JST)
> HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
> 
>> From: Wang Nan <wangnan0@huawei.com>
>> Subject: [PATCH 0/4] Replace lseek..write/read to pwrite/pread
>> Date: Sat, 26 Apr 2014 12:07:05 +0800
>> 
>> > In original code there are many operations read from /write to specific
>> > positions of a file. This series of patches replace such patterns to
>> > pread/pwrite calls, reduces more than 100 lines of code.
>> > 
>> 
>> I'm now writing pthread support patch set and it will naturally
>> include pread/pwrite like this patch set.
>> 
>> It sounds to me that using pread/pwrite only to reduce lseek code is
>> weak in motivation. Is there another visible merit? For example, any
>> kind of performance improvement. I guess it's small even if exists
>> compared to I/O.
> 
> There is no user-visible benefit just from applying the patch, that's
> right.
> 
> The main benefit is that these pread/pwrite operations are atomic and do
> not move the file offset, so all subprocesses (or threads) can share
> the same file descriptor. This allows to remove reopen_dump_memory(),
> for example.
> 
> Anyway, is improving code readability really so weak argument?
> 
> Petr T

In the current code, parallelism occurs only when --split option is
specified. It seems to me that reopen_dump_memory() is enough. So, it
appears to me that this patch set just changes lseek(); read(); or
lseek(); write() lines to pread() or pwrite(), so I don't see so big
difference...

--
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread

[Petr Tesarik]

On Wed, 30 Apr 2014 21:19:55 +0900 (JST)
HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:

> From: Petr Tesarik <ptesarik@suse.cz>
> Subject: Re: [PATCH 0/4] Replace lseek..write/read to pwrite/pread
> Date: Wed, 30 Apr 2014 13:53:04 +0200
> 
> > On Wed, 30 Apr 2014 20:41:38 +0900 (JST)
> > HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
> > 
> >> From: Wang Nan <wangnan0@huawei.com>
> >> Subject: [PATCH 0/4] Replace lseek..write/read to pwrite/pread
> >> Date: Sat, 26 Apr 2014 12:07:05 +0800
> >> 
> >> > In original code there are many operations read from /write to specific
> >> > positions of a file. This series of patches replace such patterns to
> >> > pread/pwrite calls, reduces more than 100 lines of code.
> >> > 
> >> 
> >> I'm now writing pthread support patch set and it will naturally
> >> include pread/pwrite like this patch set.
> >> 
> >> It sounds to me that using pread/pwrite only to reduce lseek code is
> >> weak in motivation. Is there another visible merit? For example, any
> >> kind of performance improvement. I guess it's small even if exists
> >> compared to I/O.
> > 
> > There is no user-visible benefit just from applying the patch, that's
> > right.
> > 
> > The main benefit is that these pread/pwrite operations are atomic and do
> > not move the file offset, so all subprocesses (or threads) can share
> > the same file descriptor. This allows to remove reopen_dump_memory(),
> > for example.
> > 
> > Anyway, is improving code readability really so weak argument?
> > 
> > Petr T
> 
> In the current code, parallelism occurs only when --split option is
> specified. It seems to me that reopen_dump_memory() is enough. So, it
> appears to me that this patch set just changes lseek(); read(); or
> lseek(); write() lines to pread() or pwrite(), so I don't see so big
> difference...

True. But as you can see in patch 4/4, it also allowed to reduce the
number of error paths.

But I'm confused. Since you say you plan to replace lseek+read/write
with pread/pwrite as part of a pthread-enabling patch series anyway,
what is your point if you argue against replacing it now?

Just trying to understand...

Petr T

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS

[Wang Nan]

On 2014/4/30 19:55, HATAYAMA Daisuke wrote:
> From: Wang Nan <wangnan0@huawei.com>
> Subject: [PATCH 3/4] makedumpfile: add -D_GNU_SOURCE to CFLAGS
> Date: Sat, 26 Apr 2014 12:07:08 +0800
> 
>> This patch is preparation for introduce pread/pwrite.
>>
> 
> Do you explain more about _GNU_SOURCE? Did you need to define this on
> your environment to build makedumpfile with pread/pwrite?
> 
> I tried to build a very simple test program using pread like
> 
> int main(void)
> {
>    printf("%p\n", pread);
> }
> 
> on RHEL5.4, RHEL6.5 and fc20, and all were done successfully without
> _GNU_SOURCE. They are all on x86_64.
> 
> I checked man pread and man pwrite on each environments for
> _GNU_SOURCE but I didn't find it. What I found was _XOPEN_SOURCE
> description only. For example this is man pread on RHEL6.5.
> 
> $ LANG=C man pread
> PREAD(2)                   Linux Programmer's Manual                  PREAD(2)
> 
> NAME
>        pread, pwrite - read from or write to a file descriptor at a given offset
> 
> SYNOPSIS
>        #define _XOPEN_SOURCE 500

I define _GNU_SOURCE because it introduces _XOPEN_SOURCE. It also introduces others which
I thought may help further improvements.

Do you mean use only _XOPEN_SOURCE for pread/pwrite?

> 
>        #include <unistd.h>
> 
>        ssize_t pread(int fd, void *buf, size_t count, off_t offset);
> 
>        ssize_t pwrite(int fd, const void *buf, size_t count, off_t offset);
> 
> Note that just as I said the above, building was successfully done on
> this environment.
> 
> --
> Thanks.
> HATAYAMA, Daisuke
> 



_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec