[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Crash-utility] add support for incomplete elf dump file



On 10/28/2014 01:35 AM, Dave Anderson wrote:

A couple notes and questions re: your latest patch:

diff --git a/diskdump.c b/diskdump.c
index 1d6f7a5..850b174 100644
--- a/diskdump.c
+++ b/diskdump.c
@@ -999,6 +999,17 @@ cache_page(physaddr_t paddr)
  	if (FLAT_FORMAT()) {
  		if (!read_flattened_format(dd->dfd, pd.offset, dd->compressed_page, pd.size))
  			return READ_ERROR;

What about FLAT_FORMAT()?  Should the (is_incomplete_dump()/pd.offset==0) test below be
done before the FLAT_FORMAT() check?

In makedumpfile, we can't set incomplete flag in flat_format. So, the patch can't support the
flat_format.


+	} else if (is_incomplete_dump()&&  (0 == pd.offset)) {

Is this how the makedumpfile patch marks pages that have been truncated, i.e., by
creating a page_desc_t that has a pd.offset == 0?

Actually, we just set incomplete flag in kdump part. When ENOSPACE happens, page_desc_t, haven't be
created, will not be created any more. And there is nothing in the position belong to it. When read
page_desc_t out at the position, pd.offset will == 0.

+		/*
+		 *  if --zero_excluded is specified and incomplete flag is set in dump file,
+		 *  zero will be used to fill the lost part.
+		 */
+		if (!(*diskdump_flags&  ZERO_EXCLUDED))
+			return READ_ERROR;
+		error(WARNING, "%s: data may be lost\n"
+			"                      pfn:%lld\n\n"
+			,pc->dumpfile,pfn);

When --zero_excluded is entered on the crash command line, then the user knows exactly
what he is doing, and so there should not be any error/warning messages from the read
command itself.  A message should only be displayed if debug mode is turned on.  For
example, this is how it is done currently, but only if debug is set to 8:

                 if (CRASHDEBUG(8))
                         fprintf(fp, "read_diskdump: zero-fill: "
                             "paddr/pfn: %llx/%lx\n",
                                 (ulonglong)paddr, pfn);


+		memset(dd->compressed_page, 0, dd->block_size);
  	} else {
  		if (lseek(dd->dfd, pd.offset, SEEK_SET) == failed)
  			return SEEK_ERROR;
diff --git a/netdump.c b/netdump.c
index abc85e0..c8f057e 100644
--- a/netdump.c
+++ b/netdump.c
@@ -608,7 +608,21 @@ read_netdump(int fd, void *bufptr, int cnt, ulong addr, physaddr_t paddr)
  				    "offset: %llx\n", (ulonglong)offset);
  			return SEEK_ERROR;
  		}
-		if (read(nd->ndfd, bufptr, cnt) != cnt) {
+		/*
+ 		 *  if --zero_excluded is specified and incomplete flag is set in ELF file,
+		 *  zero will be used to fill the lost part.
+		 */
+		ssize_t read_ret = read(nd->ndfd, bufptr, cnt);
+		if (read_ret != cnt) {
+			if (is_incomplete_dump()&&  (read_ret>= 0&&
+					*diskdump_flags&  ZERO_EXCLUDED)) {
+				error(WARNING, "%s: data may be lost\n"
+					"                   offset:%llx\n\n",
+					pc->dumpfile, offset);

Same thing here... if a user enters --zero_excluded on the command line, he should
expect strange behavior as a result.  Intermingling a bunch of WARNING messages
like the one above will only make it more confusing.

+				bufptr += read_ret;
+				bzero(bufptr, cnt - read_ret);
+				return cnt;
+			}
  			if (CRASHDEBUG(8))
  				fprintf(fp, "read_netdump: READ_ERROR: "
  				    "offset: %llx\n", (ulonglong)offset);


I fixed that. And add more descriptions to the option --zero_excluded.

Thanks
Zhou Wenjian


diff --git a/crash.8 b/crash.8
index 16f2e8d..9fef355 100644
--- a/crash.8
+++ b/crash.8
@@ -396,8 +396,9 @@ header.
 .TP
 .B --zero_excluded
 If a kdump dumpfile has been filtered to exclude various types of non-essential
-pages, any attempt to read them will fail.  With this flag,
-reads from any of those pages will return zero-filled memory.
+pages, any attempt to read them will fail. If dumpfile is incomplete, any attempt
+to read the lost pages will fail. With this flag, reads from any of those pages
+will return zero-filled memory.
 .TP
 .B --no_panic
 Do not attempt to find the task that was running when the kernel crashed.
diff --git a/diskdump.c b/diskdump.c
index 1d6f7a5..b609cd8 100644
--- a/diskdump.c
+++ b/diskdump.c
@@ -999,6 +999,17 @@ cache_page(physaddr_t paddr)
 	if (FLAT_FORMAT()) {
 		if (!read_flattened_format(dd->dfd, pd.offset, dd->compressed_page, pd.size))
 			return READ_ERROR;
+	} else if (is_incomplete_dump() && (0 == pd.offset)) {
+		/*
+		 *  if incomplete flag is set in dump file, zero will be used to fill
+		 *  the lost part.
+		 */
+		if (!(*diskdump_flags & ZERO_EXCLUDED))
+			return READ_ERROR;
+		if (CRASHDEBUG(8))
+			fprintf(fp, "cache_page: %s: data may be lost\n"
+				"zero-fill: pfn: %llx\n\n", pc->dumpfile, pfn);
+		memset(dd->compressed_page, 0, dd->block_size);
 	} else {
 		if (lseek(dd->dfd, pd.offset, SEEK_SET) == failed)
 			return SEEK_ERROR;
diff --git a/help.c b/help.c
index 723e79e..5467489 100644
--- a/help.c
+++ b/help.c
@@ -270,7 +270,8 @@ char *program_usage_info[] = {
     "",
     "  --zero_excluded",
     "    If a kdump dumpfile has been filtered to exclude various types of",
-    "    non-essential  pages, any attempt to read them will fail.  With",
+    "    non-essential  pages, any attempt to read them will fail. If dumpfile",
+    "    is incomplete, any attempt to read the lost pages will fail.  With",
     "    this flag, reads from any of those pages will return zero-filled",
     "    memory.",
     "",
@@ -1043,8 +1044,8 @@ char *help_set[] = {
 "                               (scrolling is turned off if silent is on)",
 "            edit  vi | emacs   set line editing mode (from .%src file only).",
 "        namelist  filename     name of kernel (from .%src file only).",
-"   zero_excluded  on | off     controls whether excluded pages from a dumpfile",
-"                               should return zero-filled memory.",
+"   zero_excluded  on | off     controls whether excluded pages or lost pages",
+"                               from a dumpfile should return zero-filled memory.",
 "       null-stop  on | off     if on, gdb's printing of character arrays will",
 "                               stop at the first NULL encountered.", 
 "             gdb  on | off     if on, the %s session will be run in a mode",
diff --git a/netdump.c b/netdump.c
index abc85e0..dce523d 100644
--- a/netdump.c
+++ b/netdump.c
@@ -608,7 +608,22 @@ read_netdump(int fd, void *bufptr, int cnt, ulong addr, physaddr_t paddr)
 				    "offset: %llx\n", (ulonglong)offset);
 			return SEEK_ERROR;
 		}
-		if (read(nd->ndfd, bufptr, cnt) != cnt) {
+		/*
+ 		 *  if incomplete flag is set in ELF file, zero will be used to fill
+		 *  the lost part.
+		 */ 
+		ssize_t read_ret = read(nd->ndfd, bufptr, cnt);
+		if (read_ret != cnt) {
+			if (is_incomplete_dump() && (read_ret >= 0) &&
+					(*diskdump_flags & ZERO_EXCLUDED)) {
+				if (CRASHDEBUG(8))
+					fprintf(fp, "read_netdump: %s: data may be lost\n"
+						"zero-fill:offset:%llx\n\n",
+							pc->dumpfile, offset);
+				bufptr += read_ret;
+				bzero(bufptr, cnt - read_ret);
+				return cnt;
+			}
 			if (CRASHDEBUG(8))
 				fprintf(fp, "read_netdump: READ_ERROR: "
 				    "offset: %llx\n", (ulonglong)offset);

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]