Re: [Crash-utility] [PATCH] search: Introduce -T option
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.
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. If I understand correctly, under a live dump the
panic_threads[] should be populated. So it should be safe to use?
It would also keep the searchinfo initializations contained within
the cmd_search() function like all of the other arguments.
How about the following then:
Attachments:
- 0001-search-Introduce-T-option.patch (text/plain — 5.0 KB)