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

[paawan oza]

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 at yahoo.com> 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 at redhat.com> wrote:
 

 

----- Original Message -----
> Hi Dave,
> 
> please consider V2, I tried to address your comments.
> 
> Oleg.

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 at redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

   

  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20160430/3e030793/attachment.htm>