| Message ID | 20180124094254.6171-1-richard_c_haines@btinternet.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Jan 24, 2018 at 1:42 AM, Richard Haines <richard_c_haines@btinternet.com> wrote: > Allow the tmp build files to be kept for debugging when a policy > build fails. > > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> > --- > 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_TOPLEVEL)); > 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. Bill
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@btinternet.com> wrote: > > Allow the tmp build files to be kept for debugging when a policy > > build fails. > > > > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> > > --- > > 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.
Thanks, applied: https://github.com/SELinuxProject/selinux/pull/76 On Thu, Jan 25, 2018 at 10:49 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > 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@btinternet.com> wrote: >> > Allow the tmp build files to be kept for debugging when a policy >> > build fails. >> > >> > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> >> > --- >> > 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. >
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_TOPLEVEL)); 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 */
Allow the tmp build files to be kept for debugging when a policy build fails. Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> --- 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(-)