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

[Dave Anderson <anderson@xxxxxxxxxx>]

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