<html> <head> <style></style></head> <body class='hmmessage'><div dir='ltr'><br><br><div>> Date: Fri, 19 Jun 2015 12:23:39 -0400<br>> From: anderson@redhat.com<br>> To: crash-utility@redhat.com<br>> Subject: Re: [Crash-utility] [PATCH v2] files: support dump file pages from its address space<br>> <br>> > > Here's what I suggest. Instead of using RADIX_TREE_DUMP, create a new flag<br>> > > RADIX_TREE_DUMP_CALLBACK, and you can store the callback function address inside<br>> > > the general purpose pc->curcmd_private member.<br>> > <br>> > I'm ok with use a different flag. Although I didn't find any source codes are<br>> > using RADIX_TREE_DUMP flag.<br>> > <br>> > But I had some concerns for storing callback address in pc->curcmd_private.<br>> > <br>> > I noticed that the pc->curcmd_private is already used by other crash code at<br>> > many locations.<br>> <br>> Right, but only within the context of specific commands, such as "irq -u", "vm -M",<br>> "search", "struct -f" and "runq -c". Similar to using GETBUF(), the pc->curcmd_private<br>> is only in effect within the period of time a particular command is running, and<br>> is cleared by restore_sanity() prior to the next "crash> " prompt.<br><span style="font-size: 12pt;">> > </span></div><div>> > If someday people want to use the RADIX_TREE_DUMP_CALLBACK at that context,<br>> > it may cause the troubles.<br>> <br>> How? If they want to do so within the context of a different command, they<br>> can set the pc->curcmd_private field to their own function. That's what <br>> it's designed to be used for.</div><div><br></div><div>The do_radix_tree is a general purpose api, but if we wanted to use it in "irq -u" "search"</div><div>command, we would add the limitations here. The pc->curcmd_private already used in these</div><div>commands.<br><br>> > If we really don't want to add new args for do_radix_tree, maybe we can<br>> > define a new global function pointer for this purpose.<br>> > <br>> > If you don't like my suggestion, we also can consider to use RADIX_TREE_SEARCH.<br>> <br>> Yes I suppose you could define a global pointer, but it doesn't offer any more<br>> benefit or safety than using pc->curcmd_private. You could use RADIX_TREE_SEARCH, <br>> or perhaps RADIX_TREE_COUNT/RADIX_TREE_GATHER. But both of those options would <br>> seemingly be inefficient for large lists? Having an option that utilizes a callback<br>> function does seem to be the best way to do it.<br>> <br>> How about this: have your new RADIX_TREE_DUMP_CALLBACK option use the rtp->value<br>> (void *) member to store the callback function? As long as it's documented, I would<br>> have no problem accepting that as well.<br></div><div><br></div><div>I think this is a good idea.</div><div><br></div><div>Below is my plan, </div><div><br></div><div>a. add the <span style="font-size: 12pt;">RADIX_TREE_DUMP_CB flag for do_radix_tree api.</span></div><div>b. Change the rtp definition, use union to replace original void * value definition.</div><div><br></div><div>I think pre-build extension should be able to work with new code changes.</div><div>The union definition could make api easy to be understand.</div><div><br></div><div>Below is the interface changes, could you review it in advance?</div><div><br></div><div><div>diff --git a/defs.h b/defs.h</div><div>index 95d5d92..ac97dec 100644</div><div>--- a/defs.h</div><div>+++ b/defs.h</div><div>@@ -4748,11 +4748,15 @@ int is_readable(char *);</div><div> #define RADIX_TREE_SEARCH (2)</div><div> #define RADIX_TREE_DUMP (3)</div><div> #define RADIX_TREE_GATHER (4)</div><div>+#define RADIX_TREE_DUMP_CB (5)</div><div> struct radix_tree_pair {</div><div> ulong index;</div><div>- void *value;</div><div>+ union {</div><div>+ void *value;</div><div>+ int (*cb)(ulong);</div><div>+ } entry;</div><div> };</div><div>-ulong do_radix_tree(ulong, int, struct radix_tree_pair *, int (*)(ulong));</div><div>+ulong do_radix_tree(ulong, int, struct radix_tree_pair *);</div><div> int file_dump(ulong, ulong, ulong, int, int);</div></div> </div></body> </html>