Re: [Crash-utility] PATCH v2 00/10] teach crash to work with "live" ramdump

Hi Oleg, Just was curious, what kind of addition your patch is bringing ? Broadcom brought the support for raw ramdump, well you know justpreparing ELF header and sparse support. so just wanted to understand how what you patch does, it addresses live dump ? could you elaborate on it ? Regards,Oza. On Saturday, 30 April 2016, 1:27, Dave Anderson <anderson(a)redhat.com&gt; wrote: ----- Original Message ----- Hi Oleg, This looks pretty good, but I do have a couple of questions/comments. In [PATCH v2 02/10]: @@ -124,6 +124,7 @@ fd_init(void)                 if (!pc->dumpfile) {                         pc->flags |= LIVE_SYSTEM; +                      pc->flags2 |= MEMSRC_LOCAL;                         get_live_memory_source();                 } Now that MEMSRC_LOCAL has been effectively moved out of the way, why is it being set above in fd_init()? And in [PATCH v2 10/10]: @@ -428,6 +428,17 @@ main(int argc, char **argv)                                         "too many dumpfile arguments\n");                                         program_usage(SHORT_FORM);                         } + +                      if (ACTIVE()) { +                              pc->flags |= LIVEDUMP; +                              /* disable get_live_memory_source() logic in fd_init() */ +                              pc->dumpfile = "livedump"; +                              pc->readmem = read_ramdump; +                              pc->writemem = NULL; +                              optind++; +                              continue; +                      } The pc->dumpfile should point to "/tmp/MEM" or however it was invoked on the command line, i.e., just like any other dumpfile, regardless of whether it is live or not. And if pc->dumpfile were changed to be the actual filename, then the following qualifier shouldn't be necessary: @@ -209,7 +209,8 @@ memory_source_init(void)         }         if (pc->dumpfile) { -              if (!file_exists(pc->dumpfile, NULL)) +              if (!(pc->flags & LIVEDUMP) && +                  !file_exists(pc->dumpfile, NULL))                         error(FATAL, "%s: %s\n", pc->dumpfile,                                 strerror(ENOENT)); And this change in [PATCH vs 9/10]: @@ -219,8 +209,7 @@ char *ramdump_to_elf(void)         load = (Elf64_Phdr *)ptr; -      if (alloc_program_headers(load)) -              goto end; +      alloc_program_headers(load);         offset += sizeof(Elf64_Phdr) * nodes;         ptr += sizeof(Elf64_Phdr) * nodes; causes this compiler warning if "make warn" is used: $ make warn ... [ cut ] ... cc -c -g -DX86_64 -DLZO -DSNAPPY -DGDB_7_6  ramdump.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security ramdump.c: In function 'ramdump_to_elf': ramdump.c:230:1: warning: label ‘end’ defined but not used [-Wunused-label] end: ^ ... We'll also need to vet every instance of ACTIVE() to make sure there are no other dependencies on the local system.  For example, this one comes to mind, where x86_64_calc_phys_base() looks at /proc/iomem:         if (ACTIVE()) {                 if ((iomem = fopen("/proc/iomem", "r")) == NULL)                     return;         ... And if you bring up a cscope session, and search for the "/proc/" strings, there may be other local /proc filesystem references that aren't obviously inside of "if (ACTIVE())".  I did check some of them, and most would only be relevant/called if it's a local active instance, but some may not.  But anyway, this looks good for starters. Talk to you next week,   Dave -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility