[Crash-utility] [PATCH 1/2] tree: no need to bail out on tree corruption, skip the node and move on instead

[Dave Anderson]

----- Original Message -----
> Hi Dave,
> 
> On Thu, Apr 12, 2018 at 3:32 PM, Dave Anderson <anderson at redhat.com> wrote:
> >
> > Hi Daniel,
> >
> > If the tree is corrupted, I would think that you would want to know the
> > specifics, i.e., aside from the somewhat cryptic readmem() error message.
> >
> > Perhaps it would be worth printing a more meaningful error message,
> > something like:
> >
> >   tree: rb_node: <address>: corrupt rb_left pointer: <address>
> 
> Yeah, that's a good point. I just tried a simple
> s/FAULT_ON_ERROR/RETURN_ON_ERROR/ and checked if that works, but
> keeping the former behaviour with regards to error reporting.
> 
> But having a meaningful error output would be nice. Do you prefer a
> followup patch or amend to this one?

Given my subsequent bitching about 2/2, how about a v2 patchset with both
patches updated?

Thanks,
  Dave


> 
> --nX
> 
> > That way you could do the 2 readmem() calls w/RETURN_ON_ERROR|QUIET, and
> > just have the concise error message.
> >
> > Thanks,
> >   Dave
> >
> > ----- Original Message -----
> >> ---
> >>  tools.c | 23 ++++++++++++-----------
> >>  1 file changed, 12 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/tools.c b/tools.c
> >> index 186b703463a5..992f4776281a 100644
> >> --- a/tools.c
> >> +++ b/tools.c
> >> @@ -4374,8 +4374,8 @@ rbtree_iteration(ulong node_p, struct tree_data *td,
> >> char *pos)
> >>  {
> >>       int i;
> >>       uint print_radix;
> >> -     ulong struct_p, left_p, right_p;
> >> -     char left_pos[BUFSIZE], right_pos[BUFSIZE];
> >> +     ulong struct_p, new_p;
> >> +     char new_pos[BUFSIZE];
> >>       static struct req_entry **e;
> >>
> >>       if (!node_p)
> >> @@ -4430,16 +4430,17 @@ rbtree_iteration(ulong node_p, struct tree_data
> >> *td,
> >> char *pos)
> >>               }
> >>       }
> >>
> >> -     readmem(node_p+OFFSET(rb_node_rb_left), KVADDR, &left_p,
> >> -             sizeof(void *), "rb_node rb_left", FAULT_ON_ERROR);
> >> -     readmem(node_p+OFFSET(rb_node_rb_right), KVADDR, &right_p,
> >> -             sizeof(void *), "rb_node rb_right", FAULT_ON_ERROR);
> >> -
> >> -     sprintf(left_pos, "%s/l", pos);
> >> -     sprintf(right_pos, "%s/r", pos);
> >> +     if (    readmem(node_p+OFFSET(rb_node_rb_left), KVADDR, &new_p,
> >> +                     sizeof(void *), "rb_node rb_left", RETURN_ON_ERROR))
> >> {
> >> +             sprintf(new_pos, "%s/l", pos);
> >> +             rbtree_iteration(new_p, td, new_pos);
> >> +     }
> >>
> >> -     rbtree_iteration(left_p, td, left_pos);
> >> -     rbtree_iteration(right_p, td, right_pos);
> >> +     if (    readmem(node_p+OFFSET(rb_node_rb_right), KVADDR, &new_p,
> >> +                     sizeof(void *), "rb_node rb_right",
> >> RETURN_ON_ERROR)) {
> >> +             sprintf(new_pos, "%s/r", pos);
> >> +             rbtree_iteration(new_p, td, new_pos);
> >> +     }
> >>  }
> >>
> >>  void
> >> --
> >> 2.16.2
> >>
> >> --
> >> Crash-utility mailing list
> >> Crash-utility at redhat.com
> >> https://www.redhat.com/mailman/listinfo/crash-utility
> >>
>