----- Original Message -----
> Hello Dave,
>
> I am going to fix the bug on some kernels. And I also need to confirm
> the style with you.
>
> At 2012-4-11 4:00, Dave Anderson wrote:
> > I should note that I *only* tested the "ipcs" command entered alone.
> >
> > Anyway, I now see that you separated the shared memory display into two
> > options, -m and -M. I don't believe that is necessary -- the first patch that
> > you posted gave enough basic information. If you want to expand it, perhaps the
> > extra data that is shown by "-M" option could be included in the "-i id" output?
> > And for that matter, the -u data could also be contained in the "-i id" output?
> > That way the basic shared memory data could be shown by "ipcs [-m]" option, and
> > a fully-exploded display could be done by the "-i id" qualifier because it
> > wouldn't have to be restricted to one line.
>
> I separate the display in two places beacuse displaying all items
> violates the 80-column rule. And the "-m" shows like the command
> system's build-in command. Actually, the reason why I change it like
> this is because the inode is important to my proposer. He wants to use
> it to identify memory area from such inode information. As you said, the
> information displayed by "-M" can be contained in the "-i id" output.
> But we have to get the address of inode one by one. I think debugger
> will prefer getting data at one time.
OK, then re-design it similarly to the way "kmem -s" and "kmem -S" work.
The "kmem -s" option shows a one-line summary, whereas the "kmem -S" option
shows the same one-line summary, followed by the full breakdown. The same
concept is used with "kmem -f" and "kmem -F", with "vm" and "vm -p", etc.
But the shmid_kernel, sem_array and msg_queue structure addresses should
always be shown in the one-liner.
> >
> > I also wish you had followed my original suggestion and made this into an extension
> > module first. If you do that, you could just post a single "ipcs.c" command that
> > could be dropped into the "extensions" subdirectory, and built with "make extensions".
> > There has not been a new command added to the crash utility in many years, and
> > it's going to be difficult to accept this as a built-in command until it first
> > goes through a lot of testing, usage, improvement, etc. If it were an extension
> > module, it could be stored on the extensions web page immediately for people
> > to use and test.
>
> From the aspect of myself, I would agree with it. But my proposer wants
> to use it as soon as possible as a build-in command. I am not sure how
> long it will take to convert an extension command to a build-in command.
> So I insisted sending it as a build-in command.
First -- let's be clear here -- your "proposer" does not determine whether this
command is going to be accepted as a built-in command. So when you state
that your "proposer wants to use it as soon as possible as a build-in", well,
I'm sorry, but that's just not the way things work here. I'm not in the business
of just throwing in anything that gets posted on this list into the upstream
version of the crash utility because somebody wants it as soon as possible.
And certainly your proposer can build his own version of the crash utility with
the command built-in.
To make it an extension module, the prime consideration is your usage of
MEMBER_OFFSET_INIT(), STRUCT_SIZE_INIT(), OFFSET() and SIZE(). And adapting
that should be doable by creating your own offset_table and size_table structures
containing your new entries, by doing something like this at the top of ipcs.c:
struct ipcs_offset_table {
... [ your entries ] ...
};
struct ipcs_size_table {
... [ your entries ] ...
};
#undef MEMBER_OFFSET_INIT()
#undef STRUCT_SIZE_INIT()
#define MEMBER_OFFSET_INIT() ... assign value to ipcs_offset_table.x ...
#define STRUCT_SIZE_INIT() ... assign value to ipcs_size_table.x ...
#define _OFFSET_() ... verify based upon ipcs_offset_table.x value ...
#define _SIZE_() ... verify based upon ipcs_size_table.x value ...
And then do a global search-and-replace of all of your OFFSET() and
SIZE() callers, and change them to _OFFSET_() and _SIZE_(). If you
are using any pre-existing offset_table or size_table values, then
those would obviously still need to use OFFSET() and SIZE().
Aside from that, just use "extensions/echo.c" as a template. That
way it will only consist of a single "ipcs.c" file that can be
placed into the extensions sub-directory, where "make extensions"
from the top-level crash directory will build it automatically.
Then, if the command is eventually accepted as a built-in, it will
be a simple matter of removing the stuff shown above, changing
all the _OFFSET_() and _SIZE_() callers to OFFSET() and SIZE(), and
adding the members to the offset_table and size_table structures.
Dave
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
