[Crash-utility] [Crash] [PATCH] add a new command: lscgroup

[Dave Anderson]

----- Original Message -----
> Hello Dave,
> 
> I have been working on a new command named lscgroup to
> list cgroups' information and finished it recently.
> 
> This command displays all the cgroups' information or the
> appointed cgroups' information relatively according to user's
> input. The following shows the two types of output.
> 
> 1.Display all the cgroups' information of the system.
> crash> lscgroup
> blkio:/
> blkio:/libvirt
> blkio:/libvirt/lxc
> blkio:/libvirt/qemu
> net_cls:/
> ...
> ...
> cpuset:/
> cpuset:/libvirt
> cpuset:/libvirt/lxc
> cpuset:/libvirt/qemu
> 
> 
> 2.Display the appointed cgroups' information.
> crash> lscgroup  memory:/   cpuset:/libvirt
> memory:/
> memory:/libvirt
> memory:/libvirt/lxc
> memory:/libvirt/qemu
> cpuset:/libvirt
> cpuset:/libvirt/lxc
> cpuset:/libvirt/qemu
> 
> To see more details, please refer to the help information and the patch.
> 
> To build the module from the top-level crash-<version> directory,
> enter:
>    $ cp <path-to>/lscgroup.c extensions
>    $ make extensions

Thanks for diving into this kernel subsystem.  Given the emergence
of cgroups, this command could ultimately be considered as a new 
built-in command.  

However, that being said, it does need some additional work.

First, for older kernels that don't have cgroups, there should be a
protection mechanism to prevent this from happening:

 crash> sys | grep RELEASE
      RELEASE: 2.6.18-146.el5
 crash> lscgroup

 lscgroup: invalid structure member offset: cgroupfs_root_root_list
           FILE: lscgroup.c  LINE: 503  FUNCTION: cgroup_list_cgroups()

 [/usr/bin/crash] error trace: 4611e1 => 7f3b8f3a19d7 => 7f3b8f3a14e6 => 50b701

   50b701: OFFSET_verify+161
   4611e1: exec_command+801

 lscgroup: invalid structure member offset: cgroupfs_root_root_list
           FILE: lscgroup.c  LINE: 503  FUNCTION: cgroup_list_cgroups()

 crash> 

You should check for the existence of a kernel variable or structure definition,
and if they don't exist, call command_not_supported().

Secondly, if I am not mistaken, your command only shows "enabled" cgroups?
So for example, on this RHEL6 system, the command quietly does nothing:

 crash> sys | grep RELEASE
      RELEASE: 2.6.32-15.el6.x86_64
 crash> lscgroup
 crash> 

Is that the case?  I wonder if you should also show cgroups that are 
not enabled, i.e., like /proc/cgroups does?  Or at a minimum, it should
show display something like "(no cgroups enabled)" instead of just
showing nothing at all.

Thirdly, your help page shows a "-h" option, which is unnecessary:

 crash> help lscgroup

 NAME
   lscgroup - list all cgroups

 SYNOPSIS
   lscgroup lscgroup [<controllers>:<path>] [...][-h]

  ...

because it doesn't do anything other than show the SYNOPSIS line
from the "help lscgroup" output:

 crash> lscgroup -h
 Usage:
   lscgroup lscgroup [<controllers>:<path>] [...][-h]
 Enter "help lscgroup" for details.
 crash> 

And you should change the third line in help_lscgroup[] from:

  char *help_lscgroup[] = {
  "lscgroup",
  "list all cgroups",
  "lscgroup [<controllers>:<path>] [...][-h]",

to:

  "[<controllers>:<path>] [...][-h]",

so that "lscgroup" is not printed twice in the "help lscgroup" and
the "Usage:" output.


And lastly, and most importantly, is what the command 
does *not* show.

Consider that most crash commands that delve into kernel
subsystems typically show information intermingled with
key data structure addresses.  So if there is a bug associated
with a kernel subsystem, the data structure addresses displayed
by a crash command can be used as a starting point for debugging
purposes.  

So for example, if there was a bug associated with cgroups, your
command's output only shows the name strings, but there's nothing
else to "work with" for debugging.  Wouldn't it be more helpful to
at least display the cgroup and/or the cgroupfs_root addresses?  
Then there would be useful data structure addresses to start with.

Thanks,
  Dave