On Thu, Jan 20, 2022 at 11:08 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> On Thu, Jan 13, 2022 at 6:37 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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.
