On Thu, 2018-01-25 at 10:22 -0800, William Roberts wrote:
> On Wed, Jan 24, 2018 at 1:42 AM, Richard Haines
> <richard_c_haines@xxxxxxxxxxxxxx> wrote:
> > Allow the tmp build files to be kept for debugging when a policy
> > build fails.
> >
> > Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> > ---
> > V2 Changes:
> > Remove the retain-tmp flag and just keep tmp files on build errors.
> > V3 Changes:
> > Release transaction lock after tmp files removed.
> > Add additional comments to commit_err in handle.h
> >
> > libsemanage/src/direct_api.c | 56 ++++++++++++++++++++++++++++++
> > --------------
> > libsemanage/src/handle.c | 2 ++
> > libsemanage/src/handle.h | 4 ++++
> > 3 files changed, 44 insertions(+), 18 deletions(-)
> >
> > diff --git a/libsemanage/src/direct_api.c
> > b/libsemanage/src/direct_api.c
> > index a455612f..88873c43 100644
> > --- a/libsemanage/src/direct_api.c
> > +++ b/libsemanage/src/direct_api.c
> > @@ -323,25 +323,43 @@ static void
> > semanage_direct_destroy(semanage_handle_t * sh
> > /* do nothing */
> > }
> >
> > -static int semanage_direct_disconnect(semanage_handle_t * sh)
> > +static int semanage_remove_tmps(semanage_handle_t *sh)
> > {
> > - /* destroy transaction */
> > - if (sh->is_in_transaction) {
> > - /* destroy sandbox */
> > - if (semanage_remove_directory
> > - (semanage_path(SEMANAGE_TMP,
> > SEMANAGE_TOPLEVEL)) < 0) {
> > + if (sh->commit_err)
> > + return 0;
> > +
> > + /* destroy sandbox if it exists */
> > + if (semanage_remove_directory
> > + (semanage_path(SEMANAGE_TMP, SEMANAGE_TOPLEVEL)) < 0) {
> > + if (errno != ENOENT) {
> > ERR(sh, "Could not cleanly remove sandbox
> > %s.",
> > semanage_path(SEMANAGE_TMP,
> > SEMANAGE_TOPLEVEL));
> > return -1;
> > }
> > - if (semanage_remove_directory
> > - (semanage_final_path(SEMANAGE_FINAL_TMP,
> > - SEMANAGE_FINAL_TOPLEVEL))
> > < 0) {
> > + }
> > +
> > + /* destroy tmp policy if it exists */
> > + if (semanage_remove_directory
> > + (semanage_final_path(SEMANAGE_FINAL_TMP,
> > + SEMANAGE_FINAL_TOPLEVEL)) < 0) {
> > + if (errno != ENOENT) {
> > ERR(sh, "Could not cleanly remove tmp %s.",
> > semanage_final_path(SEMANAGE_FINAL_TMP,
> > SEMANAGE_FINAL_TOPL
> > EVEL));
> > return -1;
> > }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int semanage_direct_disconnect(semanage_handle_t *sh)
> > +{
> > + int retval = 0;
> > +
> > + /* destroy transaction and remove tmp files if no commit
> > error */
> > + if (sh->is_in_transaction) {
> > + retval = semanage_remove_tmps(sh);
> > semanage_release_trans_lock(sh);
> > }
> >
> > @@ -375,7 +393,7 @@ static int
> > semanage_direct_disconnect(semanage_handle_t * sh)
> > /* Release object databases: active kernel policy */
> > bool_activedb_dbase_release(semanage_bool_dbase_active(sh))
> > ;
> >
> > - return 0;
> > + return retval;
> > }
> >
> > static int semanage_direct_begintrans(semanage_handle_t * sh)
> > @@ -1635,17 +1653,19 @@ cleanup:
> > free(mod_filenames);
> > sepol_policydb_free(out);
> > cil_db_destroy(&cildb);
> > - semanage_release_trans_lock(sh);
> >
> > free(fc_buffer);
> >
> > - /* regardless if the commit was successful or not, remove
> > the
> > - sandbox if it is still there */
> > - semanage_remove_directory(semanage_path
> > - (SEMANAGE_TMP,
> > SEMANAGE_TOPLEVEL));
> > - semanage_remove_directory(semanage_final_path
> > - (SEMANAGE_FINAL_TMP,
> > - SEMANAGE_FINAL_TOPLEVEL));
> > + /* Set commit_err so other functions can detect any errors.
> > Note that
> > + * retval > 0 will be the commit number.
> > + */
> > + if (retval < 0)
> > + sh->commit_err = retval;
> > +
> > + if (semanage_remove_tmps(sh) != 0)
> > + retval = -1;
> > +
> > + semanage_release_trans_lock(sh);
> > umask(mask);
> >
> > return retval;
> > diff --git a/libsemanage/src/handle.c b/libsemanage/src/handle.c
> > index 4ce1df03..a6567bd4 100644
> > --- a/libsemanage/src/handle.c
> > +++ b/libsemanage/src/handle.c
> > @@ -86,6 +86,8 @@ semanage_handle_t *semanage_handle_create(void)
> > * If any changes are made, this flag is ignored */
> > sh->do_rebuild = 0;
> >
> > + sh->commit_err = 0;
> > +
> > /* By default always reload policy after commit if SELinux
> > is enabled. */
> > sh->do_reload = (is_selinux_enabled() > 0);
> >
> > diff --git a/libsemanage/src/handle.h b/libsemanage/src/handle.h
> > index 1780ac81..a91907b0 100644
> > --- a/libsemanage/src/handle.h
> > +++ b/libsemanage/src/handle.h
> > @@ -62,6 +62,10 @@ struct semanage_handle {
> > int is_in_transaction;
> > int do_reload; /* whether to reload policy after
> > commit */
> > int do_rebuild; /* whether to rebuild policy if
> > there were no changes */
> > + int commit_err; /* set by semanage_direct_commit()
> > if there are
> > + * any errors when building or
> > committing the
> > + * sandbox to kernel policy at
> > /etc/selinux
> > + */
> > int modules_modified;
> > int create_store; /* whether to create the store if
> > it does not exist
> > * this will only have an effect on
> > direct connections */
> > --
> > 2.14.3
> >
> >
>
> LGTM. Stephen any other comments? I have this going through the CI
> tests as we speak.
LGTM too.
