Re: [Crash-utility] [PATCH] search: Introduce -T option
----- Original Message -----
On Wed 2017-10-04 14:52 -0400, Dave Anderson wrote:
> Couple minor things about the patch...
[ ... ]
> I'm not sure why it couldn't be used on a live dump:
>
> + if (Tflag && LIVE())
> + error(FATAL,
> + "-T option cannot be used on a live "
> + "system or live dump\n");
> +
>
> I agree that it should be restricted for a live system, but it
> seems safe to allow it for a live dump? (i.e., use ACTIVE() instead)
> With "bt", it does make sense to restrict it, because it's trying to
> unwind/translate what's being actively written on the stack when the
> dump was taken. But just a simple search should be safe.
Fair enough - I agree a 'live dump' (i.e. with pc->flags2 & LIVE_DUMP
set)
is fine for this option. I'll use ACTIVE() instead.
Actually, thinking more about it, why not just allow a search of the
active tasks? When "search -t" is used, it does search the stacks of
all active task, so I don't see why it's necessary to restrict them
for this subset. It can't hurt.
> And lastly, this minor nit -- this kind of seems like overkill:
[ ... ]
> Why not something simple like this:
>
> - if (tflag) {
> + if (tflag || Tflag) {
> searchinfo.tasks_found = 0;
> tc = FIRST_CONTEXT();
> for (i = 0; i < RUNNING_TASKS(); i++, tc++) {
> + if (Tflag && !is_task_active(tc->task))
> + continue;
> searchinfo.vaddr_start =
> GET_STACKBASE(tc->task);
> searchinfo.vaddr_end =
> GET_STACKTOP(tc->task);
> searchinfo.task_context = tc;
> searchinfo.do_task_header = TRUE;
> search_virtual(&searchinfo);
> }
> break;
> }
I don't like the above since we'd have to traverse RUNNING_TASKS() just to
find each active task.
Understood, but I have no problem with that. It's just an in-memory array,
so there's no performance or whatever issue to worry about.
If I understand correctly, under a live dump the panic_threads[]
should
be populated. So it should be safe to use?
I don't have an s390x live dump on hand, but during intialization,
panic_search() is called to populate tt->panic_threads[]:
static struct task_context *
panic_search(void)
{
struct foreach_data foreach_data, *fd;
char *p1, *p2, *tp;
ulong lasttask, dietask, found;
char buf[BUFSIZE];
struct task_context *tc;
if ((lasttask = get_dumpfile_panic_task())) {
found = TRUE;
goto found_panic_task;
}
if (pc->flags2 & LIVE_DUMP)
return NULL;
...
And of course if -T is allowed to search stacks on an active system,
it definitely won't be populated.
Dave