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