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

2018-04-15 Thread Daniel Vacek 
Hi Dave,

On Thu, Apr 12, 2018 at 3:32 PM, Dave Anderson  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: : corrupt rb_left pointer: 

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?

--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, _p,
>> - sizeof(void *), "rb_node rb_left", FAULT_ON_ERROR);
>> - readmem(node_p+OFFSET(rb_node_rb_right), KVADDR, _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, _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, _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@redhat.com
>> https://www.redhat.com/mailman/listinfo/crash-utility
>>

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

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

2018-04-12 Thread Dave Anderson 


- Original Message -
> Hi Dave,
> 
> On Thu, Apr 12, 2018 at 3:32 PM, Dave Anderson  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: : corrupt rb_left pointer: 
> 
> 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, _p,
> >> - sizeof(void *), "rb_node rb_left", FAULT_ON_ERROR);
> >> - readmem(node_p+OFFSET(rb_node_rb_right), KVADDR, _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, _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, _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@redhat.com
> >> https://www.redhat.com/mailman/listinfo/crash-utility
> >>
> 

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

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

2018-04-12 Thread Dave Anderson 


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: : corrupt rb_left pointer: 

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, _p,
> - sizeof(void *), "rb_node rb_left", FAULT_ON_ERROR);
> - readmem(node_p+OFFSET(rb_node_rb_right), KVADDR, _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, _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, _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@redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
> 

--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

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

2018-04-12 Thread Daniel Vacek 
---
 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, _p,
-   sizeof(void *), "rb_node rb_left", FAULT_ON_ERROR);
-   readmem(node_p+OFFSET(rb_node_rb_right), KVADDR, _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, _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, _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@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility