Re: [Crash-utility] [PATCH v3 1/1] tools: list: create O option for specifying head node offset (Firo Yang)

Hi, Firo Thank you for the update. On Wed, May 26, 2021 at 12:00 AM <crash-utility-request(a)redhat.com&gt; wrote: > Date: Tue, 25 May 2021 18:17:37 +0800 > From: Firo Yang <firo.yang(a)suse.com&gt; > To: k-hagio-ab(a)nec.com > Cc: firogm(a)gmail.com, crash-utility(a)redhat.com > Subject: [Crash-utility] [PATCH v3 1/1] tools: list: create O option > for specifying head node offset > Message-ID: <20210525101737.51232-1-firo.yang(a)suse.com&gt; > Content-Type: text/plain > > This -O option is very useful to specify the embedded head node's > offset which is different to the offset of other nodes embedded, > e.g. dentry.d_subdirs(the head node) and dentry.d_child. > > Signed-off-by: Firo Yang <firo.yang(a)suse.com&gt; > --- > defs.h | 1 + > help.c | 32 +++++++++++++++++++++++++++++++- > tools.c | 36 +++++++++++++++++++++++++++++++++--- > 3 files changed, 65 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..1593f12 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,15 @@ 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 Only used in conjunction with -h; it specifies the offset > of head", > +" node list_head embedded within a data structure which is > different", > +" than the offset of list_head of other nodes embedded > within a data", > +" structure.", > +" The offset may be entered in either of the following > 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 +6125,27 @@ char *help__list[] = { > " comm = \"sudo\"", > " ffff88005ac10180", > " comm = \"crash\"", > +"", > +" To display a liked list whose head node and other nodes are embedded > within ", > +" either same or different data structures resulting in different > offsets ", > +" for head node and other nodes, e.g. dentry.d_subdirs and > dentry.d_child, this", > +" -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", > NULL > }; > > diff --git a/tools.c b/tools.c > index a26b101..636adc6 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,19 @@ 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); > + if (ld->start == ld->end) { > + fprintf(fp, "(empty)\n"); > + return; > + } > The code block " if (ld->flags & LIST_HEAD_OFFSET_ENTERED) " should be at the same level with the code block "if (ld->flags & LIST_OFFSET_ENTERED)", just like the option "-o" and "-O" in the "switch-case" code blocks. For example: if (ld->flags & LIST_HEAD_POINTER) { if (!ld->end) ld->end = ld->start; readmem(ld->start + ld->member_offset, KVADDR, &ld->start, sizeof(void *), "LIST_HEAD contents", FAULT_ON_ERROR); if (ld->start == ld->end) { fprintf(fp, "(empty)\n"); return; } + } 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); + if (ld->start == ld->end) { + fprintf(fp, "(empty)\n"); + return; + } } else ld->start += ld->list_head_offset; The code looks more readable, but the above two "if" code blocks look very similar, not sure if the code refactoring will be needed. What do you think about this? Firo and Kazu.