Bug 5216

Summary: 1.20 ps lacks -w and -l options for DESKTOP case
Product: Busybox Reporter: Michael Tokarev <mjt+busybox>
Component: OtherAssignee: unassigned
Status: RESOLVED INVALID    
Severity: normal CC: busybox-cvs
Priority: P5    
Version: unspecified   
Target Milestone: ---   
Hardware: All   
OS: All   
Host: Target:
Build:

Description Michael Tokarev 2012-05-16 10:46:34 UTC
In commit 8d9ac30572818bbe78ef08d6580308e013972df3, CONFIG_FEATURE_PS_WIDE (together with newly introduced CONFIG_FEATURE_PS_LONG) has been made dependent on !CONFIG_DESKTOP.  I think the condition meant to be reverse, ie, to depend on CONFIG_DESKTOP.  Because as the result, -w and -l options are NOT available on "desktop" configuration where they should be enabled, but can be turned on/off on "non-desktop" configuration.

The attached patch changes the dependency !DESKTOP=>DESKTOP.

Actually all this !DESKTOP/DESKTOP case should provide default values instead of being listed as dependencies, but this is a different story.
Comment 1 Denys Vlasenko 2012-05-18 01:04:45 UTC
(In reply to comment #0)
> In commit 8d9ac30572818bbe78ef08d6580308e013972df3, CONFIG_FEATURE_PS_WIDE
> (together with newly introduced CONFIG_FEATURE_PS_LONG) has been made dependent
> on !CONFIG_DESKTOP.  I think the condition meant to be reverse, ie, to depend
> on CONFIG_DESKTOP.

No, in fact we have two different ps implementations, one for DESKTOP, another for !DESKTOP :(

> Because as the result, -w and -l options are NOT available
> on "desktop" configuration where they should be enabled

DESKTOP one does accept a whole slew of options, but most are ignored:

        // POSIX:
        // -a  Write information for all processes associated with terminals
        //     Implementations may omit session leaders from this list
        // -A  Write information for all processes
        // -d  Write information for all processes, except session leaders
        // -e  Write information for all processes (equivalent to -A)
        // -f  Generate a full listing
        // -l  Generate a long listing
        // -o col1,col2,col3=header
        //     Select which columns to display
        /* We allow (and ignore) most of the above. FIXME.
         * -T is picked for threads (POSIX hasn't it standardized).
         * procps v3.2.7 supports -T and shows tids as SPID column,
         * it also supports -L where it shows tids as LWP column.
         */
        opt_complementary = "o::";
        opt = getopt32(argv, "Zo:aAdefl"IF_FEATURE_SHOW_THREADS("T"), &opt_o);
        if (opt_o) {
                do {
                        parse_o(llist_pop(&opt_o));
                } while (opt_o);
        } else {
#if ENABLE_SELINUX
                if (!(opt & OPT_Z) || !is_selinux_enabled()) {
                        /* no -Z or no SELinux: do not show LABEL */
                        strcpy(default_o, DEFAULT_O_STR + sizeof(SELINUX_O_PREFIX)-1);
                } else
#endif
                {
                        strcpy(default_o, DEFAULT_O_STR);
                }
                parse_o(default_o);
        }

This else {...} block is a good place to add code which would massage default_o for -l and/or -f prior to parse_o(), making output somewhat compatible with procps. Want to prepare a patch?
Comment 2 Michael Tokarev 2012-05-23 08:39:38 UTC
I see where it goes:

#if ENABLE_SELINUX
# define SELINUX_O_PREFIX "label,"
# define DEFAULT_O_STR    (SELINUX_O_PREFIX "pid,user" IF_FEATURE_PS_TIME(",time") ",args")
#else
# define DEFAULT_O_STR    ("pid,user" IF_FEATURE_PS_TIME(",time") ",args")
#endif

...
#if ENABLE_SELINUX
                if (!(opt & OPT_Z) || !is_selinux_enabled()) {
                        /* no -Z or no SELinux: do not show LABEL */
                        strcpy(default_o, DEFAULT_O_STR + sizeof(SELINUX_O_PREFIX)-1);
                } else
#endif
                {
                        strcpy(default_o, DEFAULT_O_STR);
                }


This default_o and DEFAULT_O_STR stuff should be rearranged for this to work.  Something like,

 default_o[0] = '\0';
 if ((opt & OPT_Z) && is_selinux_enabled()) strcat(default_o,"label,");
 strcat(default_o,"pid,user"IF_FEATURE_PS_TIME(",time"));
 if (opt & OPT_L) strcat(default_o,"...");
 if (opt & OPT_W) strcat(default_o,"...");

(with proper values for "...").

But it quickly becomes rather complicated.  Also, -w is cumulative really (-www is "more w" than -w).  I'll try to cook something when I'll have time.

BTW, current busybox ps does not conform to neither POSIX nor procps in default output too.  For example, default ps shows pid, tty, time and cmd of all processes run on current tty (so listing tty column is sort of wrong), while default bysybox ps shows pid, user and cmd of all processes (user instead of tty).  Looks like if this should be fixed, it should be fixed together with -l/-w.
Comment 3 Denys Vlasenko 2012-05-27 23:10:06 UTC
(In reply to comment #2)
> I see where it goes:
> 
> #if ENABLE_SELINUX
> # define SELINUX_O_PREFIX "label,"
> # define DEFAULT_O_STR    (SELINUX_O_PREFIX "pid,user"
> IF_FEATURE_PS_TIME(",time") ",args")
> #else
> # define DEFAULT_O_STR    ("pid,user" IF_FEATURE_PS_TIME(",time") ",args")
> #endif
> 
> ...
> #if ENABLE_SELINUX
>                 if (!(opt & OPT_Z) || !is_selinux_enabled()) {
>                         /* no -Z or no SELinux: do not show LABEL */
>                         strcpy(default_o, DEFAULT_O_STR +
> sizeof(SELINUX_O_PREFIX)-1);
>                 } else
> #endif
>                 {
>                         strcpy(default_o, DEFAULT_O_STR);
>                 }
> 
> 
> This default_o and DEFAULT_O_STR stuff should be rearranged for this to work. 
> Something like,
> 
>  default_o[0] = '\0';
>  if ((opt & OPT_Z) && is_selinux_enabled()) strcat(default_o,"label,");
>  strcat(default_o,"pid,user"IF_FEATURE_PS_TIME(",time"));
>  if (opt & OPT_L) strcat(default_o,"...");
>  if (opt & OPT_W) strcat(default_o,"...");
> 
> (with proper values for "...").
> 
> But it quickly becomes rather complicated.  Also, -w is cumulative really (-www
> is "more w" than -w).  I'll try to cook something when I'll have time.

Yes, it is complicated because it tries to be space-efficient.
It will get way too ugly if we want to support -l and -f.
I recommend making it more simple even if this needs a few additional bytes.


> BTW, current busybox ps does not conform to neither POSIX nor procps in default
> output too.  For example, default ps shows pid, tty, time and cmd of all
> processes run on current tty (so listing tty column is sort of wrong), while
> default bysybox ps shows pid, user and cmd of all processes (user instead of
> tty).  Looks like if this should be fixed, it should be fixed together with
> -l/-w.

Yes, patches are welcome.
Comment 4 Denys Vlasenko 2012-09-25 18:14:19 UTC
Closing, since this is not a bug.