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