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

Hi Dave, Following the mails quiet late, and it looks like lot of discussion has gone in between you and Oleg.right now ramdump.c just addresses ARM since we work on ARM arch. if we have to support x86/_64, headers need to be dealt differently. when I had written code (before ramdump was submitted), I just wrote it with  ARM in mind,  but I think, there is a design change/tiny framework where arch specific implementation could hook their functions such as alloc_elf_headers. let me know what you think.  Regards,Oza. On Saturday, 30 April 2016, 20:39, paawan oza <paawan1982(a)yahoo.com&gt; wrote: 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