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

[yangoliver]

> 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 thesecommands.

> > If we really don't want to add new args for do_radix_tree, maybe we can
> > define a new global function pointer for this purpose.
> > 
> > If you don't like my suggestion, we also can consider to use RADIX_TREE_SEARCH.
> 
> Yes I suppose you could define a global pointer, but it doesn't offer any more
> benefit or safety than using pc->curcmd_private.  You could use RADIX_TREE_SEARCH, 
> or perhaps RADIX_TREE_COUNT/RADIX_TREE_GATHER.  But both of those options would 
> seemingly be inefficient for large lists?  Having an option that utilizes a callback
> function does seem to be the best way to do it.
> 
> 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.
Below is the interface changes, could you review it in advance?
diff --git a/defs.h b/defs.hindex 95d5d92..ac97dec 100644--- a/defs.h+++ b/defs.h@@ -4748,11 +4748,15 @@ int is_readable(char *); #define RADIX_TREE_SEARCH  (2) #define RADIX_TREE_DUMP    (3) #define RADIX_TREE_GATHER  (4)+#define RADIX_TREE_DUMP_CB (5) struct radix_tree_pair {        ulong index;-       void *value;+       union {+               void *value;+               int (*cb)(ulong);+       } entry; };-ulong do_radix_tree(ulong, int, struct radix_tree_pair *, int (*)(ulong));+ulong do_radix_tree(ulong, int, struct radix_tree_pair *); int file_dump(ulong, ulong, ulong, int, int); 		 	   		  
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20150623/e86bb2a2/attachment.htm>