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

[Dave Anderson]

----- Original Message -----
> Hi Dave,
> 
> On 04/29, Dave Anderson wrote:
> >
> > @@ -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()?
> 
> Hmm. But where else? I do not mind to change, but fd_init() looks like a
> most natural place to me. In any case it should be already set before
> fd_init() checks LOCAL_ACTIVE() (changed by the next patch) before
> match_proc_version(), 11 lines below in the same branch.

MEMSRC_LOCAL has always only been set in remote_fd_init() to signal that, for
whatever reason, the memory source is local but the vmlinux is remote.  But 
remoted_fd_init() would only be called if the deprecated remote crash daemon
was actually running on another machine and a command line argument specified it.
So for all practical purposes MEMSRC_LOCAL is an unused no-op, and should not
be set above.  

Dave

> 
> 
> 
> > +                               pc->dumpfile = "livedump";
> >
> > 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))
> 
> Yeeesss, but... OK. I'll change this in v3, but let me explain why I didn't do
> it this way.
> 
> To remind, qemu can have multiple memory-backend-file options, and in this case
> pc->dumpfile will point to the first file.
> 
> This is not really bad, and this is (almost) cosmetic. Yet I think it would be
> better to also set "pc->flags2 |= RAMDUMP" to make is_ramdump_image() == T, in
> this case and then show_ramdump_files() could show the list of files we use.
> This needs some (cosmetic) changes in show_ramdump_files() and
> display_sys_stats().
> 
> This is what V1 did, but I dropped these bits to simplify this series.
> 
> Nevermind, I'll change V3 to set
> 
> 	pc->dumpfile = ramdump[0].path
> 
> as you suggest. Then we will see, this is really minor.
> 
> 
> 
> > @@ -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:
> > ramdump.c:230:1: warning: label ‘end’ defined but not used [-Wunused-label]
> 
> Thanks! fixed.
> 
> 
> 
> > 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:
> 
> Ah. Yes, yes, yes. As I said from the very beginning, we probably need more
> LOCAL_ACTIVE() changes, so far I only tried to audit kernel.c and task.c.
> 
> And unless you feel strongly about it, I'd prefer to do this later. After
> you apply these initial changes, or at least after you fully agree with what
> I have already sent.
> 
> Thanks for your review Dave.
> 
> Oleg.
> 
>