[Crash-utility] [PATCH] add -s option for struct command

[Dave Anderson]

----- Original Message -----
> Hello Dave,
> 
> After some many discussions, I rewrite the code to an extension module.
> What I want is not only the member of structure page, but also other
> structure's members. So for the speed of analysing a big number of data
> and not obeying your attitude towards the change of struct command, I
> made it an extension module.

Thank you -- I do appreciate that.  

I will post "pstruct.c" on the extensions web page when crash-6.0.7 is released,
and will note the "crash-6.0.7 or later" restriction in the "comments" section
of its entry. 
 
> And about the patch which changes gdb, I create a new method
> print_command_2 to do the work.

That addition looks fine to me, and does not affect any other commands.
Queued for crash-6.0.7.

Also, I should note that I have incorporated your "ipcs" command into
crash-6.0.7.  It is essentially the same as your extension module, with
these exceptions:

 (1) I have re-written the "help" page

 (2) I replaced your default usage of the nsproxy of the CURRENT_TASK()
     with that of pid 0, because the command fails if the current
     task is exiting.

 (3) Similarly to the "mount" command, I have added a "-n [pid|task]" 
     namespace option:

      For kernels supporting namespaces, the -n option may be used to
      display the IPC facilities with respect to the namespace of a
      specified task:

      -n pid   a process PID.
      -n task  a hexadecimal task_struct pointer.

So congratulations are in order -- it is the first new command since 2001!

Thanks,
  Dave
 

> 
> At 2012-4-19 20:50, Dave Anderson wrote:
> >
> >
> > ----- Original Message -----
> >> At 2012-4-18 21:21, Dave Anderson wrote:
> >>> In our original discussions, i thought that I had made it clear
> >>> that
> >>> the introduction of a new option paradigm with submembers could
> >>> be
> >>> avoided by using, for example, "page._mapcounter" instead of
> >>> having
> >>> to enter "page._mapcount.counter"?  This option makes the struct
> >>> command seemingly violate its own rules, and really confuses
> >>> things.
> >>> For example, with your patch, a user would see things like this:
> >>
> >> The most important reason why I insisted this option is the
> >> performance.
> >> Both original struct and print command are very slow. When kernel
> >> debugger wants to parse a bit amount of data, the performance of
> >> original struct and print command is not ideal.
> >>
> >>>
> >>>    crash>   page._mapcount.counter ffffea0000000508 -s
> >>>    -1
> >>>    crash>   page._mapcount.counter ffffea0000000508
> >>>    struct: invalid format: page._mapcount.counter
> >>>    crash>   page._mapcount ffffea0000000508
> >>>        _mapcount = {
> >>>          counter = -1
> >>>        }
> >>>    crash>   page._mapcount ffffea0000000508 -s
> >>>    struct: invalid data structure reference page._mapcount
> >>>    crash>
> >>
> >> An idea of solving this confusion is changing the error
> >> information.
> >> When users uses "-s" option and error happens, error information
> >> suggests to use struct command without "-s" option if it is valid.
> >> And vice versa, when error happens without specified "-s" option.
> >
> > It's not so much the error message wording, it's the usage of a
> > completely different option-expression.  And you can still display
> > the -s information without the extra submember.
> >
> >>>
> >>> I had suggested that you look into the get_member_data() function
> >>> in to the gdb/symtab.c file to access the member offset and size
> >>> values.
> >>
> >> Actually, the function need to be changed a lot to support what I
> >> want.
> >> I need the information of submember, and I need the position and
> >> size of
> >> bitfield. After investigation, function print_command_1 hides the
> >> data
> >> that I want. I know it is not a good idea of modifying this
> >> function.
> >> But what if a new function which has the similar mechanism with
> >> function
> >> print_command_1?
> >
> > Right, that's exactly what I suggested below:
> >
> >>> I also don't like the idea of modifying the prototype of
> >>> the stalwart print_command_1() gdb function, and the creation
> >>> of a new gdb command.  Whenever there is a need to update the
> >>> embedded gdb version, patches like this can be problematic.
> >>> I would prefer that you create a new "GNU_XXXX" #define,
> >>> similar to GNU_GET_SYMBOL_TYPE, pass the request through
> >>> the gdb_command_funnel switch statement, and write a new
> >>> standalone function to accomplish what you have done in the
> >>> print_command_1() function.
> >
> > Dave
> >
> > --
> > Crash-utility mailing list
> > Crash-utility at redhat.com
> > https://www.redhat.com/mailman/listinfo/crash-utility
> >
> >
> 
> 
> --
> --
> Regards
> Qiao Nuohan
> 
> 
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
>