[Crash-utility] [PATCH] search: Introduce -T option

[Dave Anderson]

----- 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