[Crash-utility] [PATCH v2] files: support dump file pages from its address space

[Dave Anderson]

----- Original Message -----
> 
> 
> > Date: Fri, 19 Jun 2015 12:23:39 -0400
> > From: anderson at redhat.com
> > To: crash-utility at redhat.com
> > Subject: Re: [Crash-utility] [PATCH v2] files: support dump file pages from
> > its address space
> > 
> > > > Here's what I suggest. Instead of using RADIX_TREE_DUMP, create a new
> > > > flag
> > > > RADIX_TREE_DUMP_CALLBACK, and you can store the callback function
> > > > address inside
> > > > the general purpose pc->curcmd_private member.
> > > 
> > > I'm ok with use a different flag. Although I didn't find any source codes
> > > are
> > > using RADIX_TREE_DUMP flag.
> > > 
> > > But I had some concerns for storing callback address in
> > > pc->curcmd_private.
> > > 
> > > I noticed that the pc->curcmd_private is already used by other crash code
> > > at
> > > many locations.
> > 
> > Right, but only within the context of specific commands, such as "irq -u", "vm -M",
> > "search", "struct -f" and "runq -c". Similar to using GETBUF(), the pc->curcmd_private
> > is only in effect within the period of time a particular command is running, and
> > is cleared by restore_sanity() prior to the next "crash> " prompt.
> > > 
> > > If someday people want to use the RADIX_TREE_DUMP_CALLBACK at that context,
> > > it may cause the troubles.
> > 
> > How? If they want to do so within the context of a different command, they
> > can set the pc->curcmd_private field to their own function. That's what
> > it's designed to be used for.
> 
> The do_radix_tree is a general purpose api, but if we wanted to use it in "irq -u" "search"
> command, we would add the limitations here. The pc->curcmd_private already used in these
> commands.

OK, yes, in that case, point taken...

> 
> > 
> > How about this: have your new RADIX_TREE_DUMP_CALLBACK option use the rtp->value
> > (void *) member to store the callback function? As long as it's documented, I would
> > have no problem accepting that as well.
> 
> I think this is a good idea.
> 
> Below is my plan,
> 
> a. add the RADIX_TREE_DUMP_CB flag for do_radix_tree api.
> b. Change the rtp definition, use union to replace original void * value definition.
> 
> I think pre-build extension should be able to work with new code changes.
> The union definition could make api easy to be understand.

It may work with pre-existing extension modules, but if an extension module that uses
a radix_tree_pair structure gets re-compiled, it fails like this:

  error: 'struct radix_tree_pair' has no member named 'value'  

I should have recommended using the radix_tree_pair.value as my first suggestion,
but I was mistakenly thinking that your code was already using it.  (although even
if if you were using it, the value pointer could still be used, for example, to point
to a data structure containing a array pointer and your callback function)

So please just cast the value member to the callback function.

Thanks,
  Dave