Re: [PATCH] libselinux/utils/getsebool: add options to display en-/disabled booleans

[Petr Lautrbach <plautrba@redhat.com>]

Christian Göttsche <cgzones@googlemail.com> writes:

> On Wed, 4 May 2022 at 14:52, Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> Christian Göttsche <cgzones@googlemail.com> writes:
>>
>> > Add command line options to getsebool(8) to display either all enabled
>> > or all disabled booleans.
>>
>> I'm curious what would you use this for?
>
> I tried to list all enabled booleans and `getsebool -a | grep on`
> listed also ones with 'on' in their name.
> Using `getsebool -a | grep on'` works however, so this patch is not essential.

I see.

>>
>> Another comment is bellow
>>
>>
>> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>> > ---
>> >  libselinux/man/man8/getsebool.8 |  8 +++++++-
>> >  libselinux/utils/getsebool.c    | 36 +++++++++++++++++++++++++++------
>> >  2 files changed, 37 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/libselinux/man/man8/getsebool.8 b/libselinux/man/man8/getsebool.8
>> > index d70bf1e4..d8356d36 100644
>> > --- a/libselinux/man/man8/getsebool.8
>> > +++ b/libselinux/man/man8/getsebool.8
>> > @@ -4,7 +4,7 @@ getsebool \- get SELinux boolean value(s)
>> >  .
>> >  .SH "SYNOPSIS"
>> >  .B getsebool
>> > -.RB [ \-a ]
>> > +.RB [ \-a | \-0 | \-1 ]
>> >  .RI [ boolean ]
>> >  .
>> >  .SH "DESCRIPTION"
>> > @@ -26,6 +26,12 @@ their pending values as desired and then committing once.
>> >  .TP
>> >  .B \-a
>> >  Show all SELinux booleans.
>> > +.TP
>> > +.B \-0
>> > +Show all disabled SELinux booleans.
>> > +.TP
>> > +.B \-1
>> > +Show all enabled SELinux booleans.
>> >  .
>> >  .SH AUTHOR
>> >  This manual page was written by Dan Walsh <dwalsh@redhat.com>.
>> > diff --git a/libselinux/utils/getsebool.c b/libselinux/utils/getsebool.c
>> > index 36994536..7fb0b58b 100644
>> > --- a/libselinux/utils/getsebool.c
>> > +++ b/libselinux/utils/getsebool.c
>> > @@ -6,21 +6,31 @@
>> >  #include <string.h>
>> >  #include <selinux/selinux.h>
>> >
>> > +enum list_mode {
>> > +     SPECIFIED,
>> > +     ALL,
>> > +     DISABLED,
>> > +     ENABLED,
>> > +};
>> > +
>> >  static __attribute__ ((__noreturn__)) void usage(const char *progname)
>> >  {
>> > -     fprintf(stderr, "usage:  %s -a or %s boolean...\n", progname, progname);
>> > +     fprintf(stderr, "usage:  %s [-a|-0|-1] or %s boolean...\n", progname, progname);
>> >       exit(1);
>> >  }
>> >
>> >  int main(int argc, char **argv)
>> >  {
>> > -     int i, get_all = 0, rc = 0, active, pending, len = 0, opt;
>> > +     int i, rc = 0, active, pending, len = 0, opt;
>> > +     enum list_mode mode = SPECIFIED;
>> >       char **names = NULL;
>> >
>> > -     while ((opt = getopt(argc, argv, "a")) > 0) {
>> > +     while ((opt = getopt(argc, argv, "a01")) > 0) {
>> >               switch (opt) {
>> >               case 'a':
>> > -                     if (argc > 2)
>> > +             case '0':
>> > +             case '1':
>> > +                     if (argc > 2 || mode != SPECIFIED)
>> >                               usage(argv[0]);
>> >                       if (is_selinux_enabled() <= 0) {
>> >                               fprintf(stderr, "%s:  SELinux is disabled\n",
>> > @@ -39,7 +49,17 @@ int main(int argc, char **argv)
>> >                               printf("No booleans\n");
>> >                               return 0;
>> >                       }
>> > -                     get_all = 1;
>> > +                     switch (opt) {
>> > +                     case 'a':
>> > +                             mode = ALL;
>> > +                             break;
>> > +                     case '0':
>> > +                             mode = DISABLED;
>> > +                             break;
>> > +                     case '1':
>> > +                             mode = ENABLED;
>> > +                             break;
>> > +                     }
>>
>> switch(opt) inside switch(opt) block looks strange for me. Would it make
>> sense to have just this switch to set mode and move the code from line
>> 35 around is_selinux_enabled() and security_get_boolean_names() after the switch?
>>
>
> Yes, that would make sense; the change I made aimed for a minimal diff size.


Could you please change it?


>
>> Petr
>>
>>
>> >                       break;
>> >               default:
>> >                       usage(argv[0]);
>> > @@ -74,7 +94,7 @@ int main(int argc, char **argv)
>> >       for (i = 0; i < len; i++) {
>> >               active = security_get_boolean_active(names[i]);
>> >               if (active < 0) {
>> > -                     if (get_all && errno == EACCES)
>> > +                     if (mode != SPECIFIED && errno == EACCES)
>> >                               continue;
>> >                       fprintf(stderr, "Error getting active value for %s\n",
>> >                               names[i]);
>> > @@ -88,6 +108,10 @@ int main(int argc, char **argv)
>> >                       rc = -1;
>> >                       goto out;
>> >               }
>> > +             if ((mode == ENABLED  && active == 0 && pending == 0) ||
>> > +                 (mode == DISABLED && active == 1 && pending == 1)) {
>> > +                     continue;
>> > +             }
>> >               char *alt_name = selinux_boolean_sub(names[i]);
>> >               if (! alt_name) {
>> >                       perror("Out of memory\n");
>> > --
>> > 2.36.0
>>