Re: [RFC PATCH userspace 4/5] libsemanage: optionally rebuild policy when modules are changed externally

[Ondrej Mosnacek <omosnace@redhat.com>]

On Thu, Jan 20, 2022 at 11:08 PM James Carter <jwcart2@gmail.com> wrote:
> On Thu, Jan 13, 2022 at 6:37 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:\
[...]
> > @@ -1119,13 +1248,8 @@ static int semanage_direct_commit(semanage_handle_t * sh)
> >                 }
> >         }
> >
> > -       /* Before we do anything else, flush the join to its component parts.
> > -        * This *does not* flush to disk automatically */
> > -       if (users->dtable->is_modified(users->dbase)) {
> > -               retval = users->dtable->flush(sh, users->dbase);
> > -               if (retval < 0)
> > -                       goto cleanup;
> > -       }
> > +       if (force_rebuild)
> > +               goto rebuild;
> >
> >         /*
> >          * This is for systems that have already migrated with an older version
> > @@ -1135,70 +1259,66 @@ static int semanage_direct_commit(semanage_handle_t * sh)
> >          * in order to skip re-linking are present; otherwise, we force
> >          * a rebuild.
> >          */
> > -       if (!do_rebuild) {
> > -               int files[] = {SEMANAGE_STORE_KERNEL,
> > -                                          SEMANAGE_STORE_FC,
> > -                                          SEMANAGE_STORE_SEUSERS,
> > -                                          SEMANAGE_LINKED,
> > -                                          SEMANAGE_SEUSERS_LINKED,
> > -                                          SEMANAGE_USERS_EXTRA_LINKED};
> > -
> > -               for (i = 0; i < (int) ARRAY_SIZE(files); i++) {
> > -                       path = semanage_path(SEMANAGE_TMP, files[i]);
> > -                       if (stat(path, &sb) != 0) {
> > -                               if (errno != ENOENT) {
> > -                                       ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > -                                       retval = -1;
> > -                                       goto cleanup;
> > -                               }
> > -
> > -                               do_rebuild = 1;
> > -                               goto rebuild;
> > +       for (i = 0; i < (int) ARRAY_SIZE(semanage_computed_files); i++) {
> > +               path = semanage_path(SEMANAGE_TMP, semanage_computed_files[i]);
> > +               if (stat(path, &sb) != 0) {
> > +                       if (errno != ENOENT) {
> > +                               ERR(sh, "Unable to access %s: %s\n", path, strerror(errno));
> > +                               retval = -1;
> > +                               goto cleanup;
> >                         }
> > +
> > +                       force_rebuild = 1;
> > +                       goto rebuild;
> >                 }
> >         }
> >
> >  rebuild:
>
> I know that the rebuild label and goto rebuild was there originally,
> but I would prefer to have it eliminated.
>
> instead of:
> if (force_rebuild)
>     goto rebuild;
> ...
> for (...
>     path = ...
>     if (...
>         ...
>         force_rebuild = 1;
>         goto rebuild;
>     }
> }
>
> rebuild:
>
> why not:
> if (!force_rebuild)
>     for (...
>         path = ...
>         if (...
>             force_rebuild = 1;
>             break;
>         }
>     }
> }

Good point, it really doesn't need to be there. I'll remove it in the
next revision.

Thanks for the review!

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.