[Crash-utility] [PATCH] search: Introduce -T option
Dave Anderson
anderson at redhat.com
Wed Oct 4 18:52:26 UTC 2017
Hi Aaron,
That's a pretty good idea, I like it.
Couple minor things about the patch...
If -t cannot be used for XEN_HYPER_MODE(), then it shouldn't be allowed
for -T as well:
+ case 'T':
+ Tflag++;
+ break;
+
case 't':
if (XEN_HYPER_MODE())
error(FATAL,
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.
And lastly, this minor nit -- this kind of seems like overkill:
+static void
+prepare_searchinfo_for_task_search(struct searchinfo *si, struct task_context *tc)
+{
+ si->tasks_found = 0;
+ si->vaddr_start = GET_STACKBASE(tc->task);
+ si->vaddr_end = GET_STACKTOP(tc->task);
+ si->task_context = tc;
+ si->do_task_header = TRUE;
+}
...
+ if (Tflag) {
+ for (c = 0; c < NR_CPUS; c++)
+ if ((tc = task_to_context(tt->panic_threads[c]))) {
+ prepare_searchinfo_for_task_search(&searchinfo, tc);
+ search_virtual(&searchinfo);
+ }
+ break;
+ }
+
if (tflag) {
- searchinfo.tasks_found = 0;
tc = FIRST_CONTEXT();
for (i = 0; i < RUNNING_TASKS(); i++, tc++) {
- searchinfo.vaddr_start = GET_STACKBASE(tc->task);
- searchinfo.vaddr_end = GET_STACKTOP(tc->task);
- searchinfo.task_context = tc;
- searchinfo.do_task_header = TRUE;
+ prepare_searchinfo_for_task_search(&searchinfo, tc);
search_virtual(&searchinfo);
}
break;
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;
}
It would also keep the searchinfo initializations contained within
the cmd_search() function like all of the other arguments.
Thanks,
Dave
----- Original Message -----
> Like the -t option, except consider only the "active" task
> on each CPU and cannot be used on a live system or dump.
> The -t and -T options are mutually exclusive.
>
> Signed-off-by: Aaron Tomlin <atomlin at redhat.com>
> ---
> help.c | 4 +++-
> memory.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
> 2 files changed, 43 insertions(+), 13 deletions(-)
>
> diff --git a/help.c b/help.c
> index 2d80202..5b3e89c 100644
> --- a/help.c
> +++ b/help.c
> @@ -2862,7 +2862,7 @@ NULL
> char *help_search[] = {
> "search",
> "search memory",
> -"[-s start] [ -[kKV] | -u | -p | -t ] [-e end | -l length] [-m mask]\n"
> +"[-s start] [ -[kKV] | -u | -p | -t|-T ] [-e end | -l length] [-m mask]\n"
> " [-x count] -[cwh] [value | (expression) | symbol | string] ...",
> " This command searches for a given value within a range of user virtual,
> kernel",
> " virtual, or physical memory space. If no end nor length value is
> entered, ",
> @@ -2893,6 +2893,8 @@ char *help_search[] = {
> " -t Search only the kernel stack pages of every task. If one or
> more",
> " matches are found in a task's kernel stack, precede the
> output",
> " with a task-identifying header.",
> +" -T Same as -t, except only the active task(s) are considered.",
> +" This option cannot be used on a live system or dump.",
> " -e end Stop the search at this hexadecimal user or kernel virtual",
> " address, kernel symbol, or physical address. The end
> address",
> " must be appropriate for the memory type specified.",
> diff --git a/memory.c b/memory.c
> index 8efe0b2..573b670 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -227,6 +227,7 @@ static ulonglong search_ushort_p(ulong *, ulonglong, int,
> struct searchinfo *);
> static ulonglong search_chars_p(ulong *, ulonglong, int, struct searchinfo
> *);
> static void search_virtual(struct searchinfo *);
> static void search_physical(struct searchinfo *);
> +static void prepare_searchinfo_for_task_search(struct searchinfo *, struct
> task_context *);
> static int next_upage(struct task_context *, ulong, ulong *);
> static int next_kpage(ulong, ulong *);
> static int next_physpage(ulonglong, ulonglong *);
> @@ -13864,7 +13865,15 @@ generic_get_kvaddr_ranges(struct vaddr_range *rp)
> return cnt;
> }
>
> -
> +static void
> +prepare_searchinfo_for_task_search(struct searchinfo *si, struct
> task_context *tc)
> +{
> + si->tasks_found = 0;
> + si->vaddr_start = GET_STACKBASE(tc->task);
> + si->vaddr_end = GET_STACKTOP(tc->task);
> + si->task_context = tc;
> + si->do_task_header = TRUE;
> +}
> /*
> * Search for a given value between a starting and ending address range,
> * applying an optional mask for "don't care" bits. As an alternative
> @@ -13882,7 +13891,7 @@ cmd_search(void)
> ulong value, mask, len;
> ulong uvaddr_start, uvaddr_end;
> ulong kvaddr_start, kvaddr_end, range_end;
> - int sflag, Kflag, Vflag, pflag, tflag;
> + int sflag, Kflag, Vflag, pflag, Tflag, tflag;
> struct searchinfo searchinfo;
> struct syment *sp;
> struct node_table *nt;
> @@ -13896,7 +13905,7 @@ cmd_search(void)
>
> context = max = 0;
> start = end = 0;
> - value = mask = sflag = pflag = Kflag = Vflag = memtype = len = tflag = 0;
> + value = mask = sflag = pflag = Kflag = Vflag = memtype = len = Tflag =
> tflag = 0;
> kvaddr_start = kvaddr_end = 0;
> uvaddr_start = UNINITIALIZED;
> uvaddr_end = COMMON_VADDR_SPACE() ? (ulong)(-1) : machdep->kvbase;
> @@ -13933,7 +13942,7 @@ cmd_search(void)
>
> searchinfo.mode = SEARCH_ULONG; /* default search */
>
> - while ((c = getopt(argcnt, args, "tl:ukKVps:e:v:m:hwcx:")) != EOF) {
> + while ((c = getopt(argcnt, args, "Ttl:ukKVps:e:v:m:hwcx:")) != EOF)
> {
> switch(c)
> {
> case 'u':
> @@ -14038,6 +14047,10 @@ cmd_search(void)
> context = dtoi(optarg, FAULT_ON_ERROR, NULL);
> break;
>
> + case 'T':
> + Tflag++;
> + break;
> +
> case 't':
> if (XEN_HYPER_MODE())
> error(FATAL,
> @@ -14052,10 +14065,20 @@ cmd_search(void)
> }
> }
>
> - if (tflag && (memtype || start || end || len))
> + if ((tflag || Tflag) && (memtype || start || end || len))
> + error(FATAL,
> + "-%s option cannot be used with other "
> + "memory-selection options\n",
> + tflag ? "t" : "T");
> +
> + if (Tflag && LIVE())
> + error(FATAL,
> + "-T option cannot be used on a live "
> + "system or live dump\n");
> +
> + if (tflag && Tflag)
> error(FATAL,
> - "-t option cannot be used with other "
> - "memory-selection options\n");
> + "-t and -T options are mutually exclusive\n");
>
> if (XEN_HYPER_MODE()) {
> memtype = KVADDR;
> @@ -14328,14 +14351,19 @@ cmd_search(void)
> break;
> }
>
> + if (Tflag) {
> + for (c = 0; c < NR_CPUS; c++)
> + if ((tc = task_to_context(tt->panic_threads[c]))) {
> + prepare_searchinfo_for_task_search(&searchinfo, tc);
> + search_virtual(&searchinfo);
> + }
> + break;
> + }
> +
> if (tflag) {
> - searchinfo.tasks_found = 0;
> tc = FIRST_CONTEXT();
> for (i = 0; i < RUNNING_TASKS(); i++, tc++) {
> - searchinfo.vaddr_start = GET_STACKBASE(tc->task);
> - searchinfo.vaddr_end = GET_STACKTOP(tc->task);
> - searchinfo.task_context = tc;
> - searchinfo.do_task_header = TRUE;
> + prepare_searchinfo_for_task_search(&searchinfo, tc);
> search_virtual(&searchinfo);
> }
> break;
> --
> 2.9.4
>
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>
More information about the Crash-utility
mailing list