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

The 06/01/2021 13:01, lijiang wrote: > 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. Hi Lianbo, after thinking about this code for a while, I think the code refactoring is not necessary because 1) the original code makes more sense to express the mutual exclusive semantics between -H (ld->flags & LIST_HEAD_POINTER) and -h. 2) LIST_HEAD_OFFSET_ENTERED and LIST_OFFSET_ENTERED don't have to be at same level because LIST_HEAD_OFFSET_ENTERED is just a sub-option for -h. So please consider accepting the original patch. Thanks, Firo