The 05/25/2021 07:32, HAGIO KAZUHITO(萩尾 一仁) wrote:
Hi Kazu-san,
Thanks for reviewing.
> Hi Firo,
>
> thanks for updating the patch.
>
> -----Original Message-----
> > This new O option is very useful to specify the head node
> > offset for listing linked list whose head node embedded has a
> > different offset to other node, e.g. dentry.d_subdirs(the head node)
> > and dentry.d_child.
> >
> > Signed-off-by: Firo Yang <firo.yang@xxxxxxxx>
> > ---
> > defs.h | 1 +
> > help.c | 15 ++++++++++++++-
> > tools.c | 32 +++++++++++++++++++++++++++++---
> > 3 files changed, 44 insertions(+), 4 deletions(-)
> >
> > diff --git a/defs.h b/defs.h
> > index 396d61a..d5fcd37 100644
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -2613,6 +2613,7 @@ struct list_data { /* generic structure used by do_list() to walk */
> > #define LIST_PARSE_MEMBER (VERBOSE << 13)
> > #define LIST_READ_MEMBER (VERBOSE << 14)
> > #define LIST_BRENT_ALGO (VERBOSE << 15)
> > +#define LIST_HEAD_OFFSET_ENTERED (VERBOSE << 16)
> >
> > struct tree_data {
> > ulong flags;
> > diff --git a/help.c b/help.c
> > index e0c8408..3aa99f0 100644
> > --- a/help.c
> > +++ b/help.c
> > @@ -5716,7 +5716,7 @@ char *help__list[] = {
> > "list",
> > "linked list",
> > "[[-o] offset][-e end][-[s|S] struct[.member[,member] [-l offset]] -[x|d]]"
> > -"\n [-r|-B] [-h|-H] start",
> > +"\n [-r|-B] [-h [-O head_offset]|-H] start",
> > " ",
> > " This command dumps the contents of a linked list. The entries in a linked",
> > " list are typically data structures that are tied together in one of two",
> > @@ -5800,6 +5800,14 @@ char *help__list[] = {
> > " -S struct Similar to -s, but instead of parsing gdb output, member values",
> > " are read directly from memory, so the command works much faster",
> > " for 1-, 2-, 4-, and 8-byte members.",
>
> > +" -O offset The -O option works only with -h option.",
> > +" It is used for specifying the offset of head node embedded in a",
> > +" structure, like dentry.d_subdirs or cgroup_subsys_state.children.",
>
> I would like to describe it a bit more and generally here, something like:
> ---
> Only used in conjunction with -h, specify the offset of a head node list_head
> that is embedded within a data structure that is different than the offset of
> list_head within the other structures in the linked list.
> ---
> This might be poor, please help to edit..
More clear than mine. Will use it.
>
> > +" The offset may be entered in either of two manners:",
> > +"",
> > +" 1. \"structure.member\" format.",
> > +" 2. a number of bytes.",
> > +"",
> > " -l offset Only used in conjunction with -s, if the start address argument",
> > " is a pointer to an embedded list head (or any other similar list",
> > " linkage structure whose first member points to the next linkage",
> > @@ -6116,6 +6124,11 @@ char *help__list[] = {
> > " comm = \"sudo\"",
> > " ffff88005ac10180",
> > " comm = \"crash\"",
> > +"",
>
> > +" With the support of '-O' option, we can easily achieve the following convenient",
> > +" usage: ",
> > +" %s> alias ls list -O dentry.d_subdirs -o dentry.d_child -s dentry.d_name.name -h ",
> > +" %s> ls <address of dentry>",
>
> That alias is good but it would be better to put an actual output and
> describe the option more to understand cases that can be used.
> How about this?
> ---
> To display a list that is embedded within a data structure that is different
> than the offset of list_head within the other structures in the linked list,
> e.g. dentry.d_subdirs and dentry.d_child, the -O option can be used:
>
> %s> list -o dentry.d_child -s dentry.d_name.name -O dentry.d_subdirs -h ffff9c585b81a180
> ffff9c585b9cb140
> d_name.name = 0xffff9c585b9cb178 "ccc.txt"
> ffff9c585b9cb980
> d_name.name = 0xffff9c585b9cb9b8 "bbb.txt"
> ffff9c585b9cb740
> d_name.name = 0xffff9c585b9cb778 "aaa.txt"
>
> The dentry.d_subdirs example above is equal to the following sequence:
>
> %s> struct -o dentry.d_subdirs ffff9c585b81a180
> struct dentry {
> [ffff9c585b81a220] struct list_head d_subdirs;
> }
> %s> list -o dentry.d_child -s dentry.d_name.name -H ffff9c585b81a220
> ---
More concrete than mine. Will use it.
>
> (just fyi, if you want to list directory entries and inodes, the cacheutils
> extension module may help you: https://github.com/k-hagio/crash-cacheutils)
Good to know. Thanks!
>
>
> > NULL
> > };
> >
> > diff --git a/tools.c b/tools.c
> > index a26b101..6a11670 100644
> > --- a/tools.c
> > +++ b/tools.c
> > @@ -3343,6 +3343,7 @@ void
> > cmd_list(void)
> > {
> > int c;
> > + long head_member_offset = 0; /* offset for head like denty.d_subdirs */
> > struct list_data list_data, *ld;
> > struct datatype_member struct_member, *sm;
> > struct syment *sp;
> > @@ -3353,7 +3354,7 @@ cmd_list(void)
> > BZERO(ld, sizeof(struct list_data));
> > struct_list_offset = 0;
> >
> > - while ((c = getopt(argcnt, args, "BHhrs:S:e:o:xdl:")) != EOF) {
> > + while ((c = getopt(argcnt, args, "BHhrs:S:e:o:O:xdl:")) != EOF) {
> > switch(c)
> > {
> > case 'B':
> > @@ -3394,6 +3395,24 @@ cmd_list(void)
> > optarg);
> > break;
> >
> > + case 'O':
> > + if (ld->flags & LIST_HEAD_OFFSET_ENTERED)
> > + error(FATAL,
> > + "offset value %d (0x%lx) already entered\n",
> > + head_member_offset, head_member_offset);
> > + else if (IS_A_NUMBER(optarg))
> > + head_member_offset = stol(optarg,
> > + FAULT_ON_ERROR, NULL);
> > + else if (arg_to_datatype(optarg,
> > + sm, RETURN_ON_ERROR) > 1)
> > + head_member_offset = sm->member_offset;
> > + else
> > + error(FATAL, "invalid -O argument: %s\n",
> > + optarg);
> > +
> > + ld->flags |= LIST_HEAD_OFFSET_ENTERED;
> > + break;
> > +
> > case 'o':
> > if (ld->flags & LIST_OFFSET_ENTERED)
> > error(FATAL,
> > @@ -3599,8 +3618,15 @@ next_arg:
> > fprintf(fp, "(empty)\n");
> > return;
> > }
> > - } else
> > - ld->start += ld->list_head_offset;
> > + } else {
> > + if (ld->flags & LIST_HEAD_OFFSET_ENTERED) {
> > + if (!ld->end)
> > + ld->end = ld->start + head_member_offset;
> > + readmem(ld->start + head_member_offset, KVADDR,
> > + &ld->start, sizeof(void *), "LIST_HEAD contents", FAULT_ON_ERROR);
>
> As wrote above, I understand that the following sequence
>
> crash> struct -o dentry.d_subdirs ffff9c585b81a180
> crash> list -o dentry.d_child -s dentry.d_name.name -H ffff9c585b81a220
>
> is equal to
>
> crash> list -o dentry.d_child -s dentry.d_name.name -O dentry.d_subdirs -h ffff9c585b81a180
>
> with the patch.
>
> But if the dentry.d_subdirs list is empty, the two results are not equal:
>
> crash> list -o dentry.d_child -s dentry.d_name.name -H ffff9c594fb57de0
> (empty)
> crash> list -o dentry.d_child -s dentry.d_name.name -O dentry.d_subdirs -h ffff9c594fb57d40
> ffff9c594fb57d50
> d_name.name = 0x2e63007974706d65 <Address 0x2e63007974706d65 out of bounds>
>
> So, this is also needed?
>
> --- a/tools.c
> +++ b/tools.c
> @@ -3624,6 +3624,10 @@ next_arg:
> ld->end = ld->start + head_member_offset;
> readmem(ld->start + head_member_offset, KVADDR,
> &ld->start, sizeof(void *), "LIST_HEAD contents", FAULT_ON_ERROR);
> + if (ld->start == ld->end) {
> + fprintf(fp, "(empty)\n");
> + return;
> + }
Great catch! Will also add this change to v3.
Thanks,
Firo
> } else
> ld->start += ld->list_head_offset;
> }
>
> Thanks,
> Kazu
>
> > + } else
> > + ld->start += ld->list_head_offset;
> > + }
> > }
> >
> > ld->flags &= ~(LIST_OFFSET_ENTERED|LIST_START_ENTERED);
> > --
> > 2.31.1
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
