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

Date: Fri, 19 Jun 2015 12:23:39 -0400 From: anderson(a)redhat.com To: crash-utility(a)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.

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