Re: [Crash-utility] PATCH v2 00/10] teach crash to work with "live" ramdump
----- 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.