Richard, are you going to respin this?
On Tue, Jan 16, 2018 at 9:35 AM, William Roberts
<bill.c.roberts@xxxxxxxxx> wrote:
> On Tue, Jan 16, 2018 at 8:00 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
>> On Tue, 2018-01-16 at 07:47 -0800, William Roberts wrote:
>>> On Mon, Jan 15, 2018 at 9:32 AM, Stephen Smalley
>>> <stephen.smalley@xxxxxxxxx> wrote:
>>> > On Jan 14, 2018 10:36 AM, "Richard Haines" <richard_c_haines@btinte
>>> > rnet.com>
>>> > wrote:
>>> >
>>> > Add new option to semanage.conf that allows the tmp build files
>>> > to be kept for debugging when building policy.
>>> >
>>> >
>>> > Would it be better to just retain the files by default if there is
>>> > an error?
>>>
>>> I thought about this as well, my reasoning as to why Richard's
>>> approach was
>>> better is that if someone does it N times trying to figure out an
>>> issue,
>>> then there would be N piles of files in the tmp folder. This way they
>>> have to opt in to have their tmp folder spammed.
>>
>> I believe that the tmp directories are deleted and re-created by
>> libsemanage each time before use (otherwise we'd have a different
>> problem with not removing them, since we could end up with a mix of
>> files from different, incomplete transactions being intermingled
>> there). So I don't think this would be a problem. It might however
>
> Oh I see it looks like its just generating a /tmp "store" directory
> under the semanage path. I thought that enum was triggering a true
> /tmp style thing. I should have looksed closer.
>
>> require saving the commit success/failure result in the handle so that
>> we know in semanage_direct_disconnect() whether or not we should delete
>> it.
>
> Now that I understand that tid-bit, I think you're right, let's just
> leave it on error.
>
>>
>> If we truly need to make it optional, then I'd rather have it be an
>> option flag to semodule and a runtime setting of libsemanage (ala
>> reload, disable_dontaudit, etc) than a semanage.conf setting, as this
>> is something a user will want to be able to use without having to edit
>> a config file, re-run the transaction, and then re-edit the config file
>> each time. But I'm not convinced we can't just make it the default
>> behavior whenever the commit fails. Deleting the tmp files
>> automatically only really makes sense when it succeeds.
>
>
>
>>
>>>
>>> >
>>> >
>>> > Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
>>> > ---
>>> > libsemanage/man/man5/semanage.conf.5 | 8 ++++++++
>>> > libsemanage/src/conf-parse.y | 15 ++++++++++++++-
>>> > libsemanage/src/conf-scan.l | 1 +
>>> > libsemanage/src/direct_api.c | 21 ++++++++++++---------
>>> > libsemanage/src/semanage_conf.h | 1 +
>>> > 5 files changed, 36 insertions(+), 10 deletions(-)
>>> >
>>> > diff --git a/libsemanage/man/man5/semanage.conf.5
>>> > b/libsemanage/man/man5/semanage.conf.5
>>> > index 8f8de55a..10cab65a 100644
>>> > --- a/libsemanage/man/man5/semanage.conf.5
>>> > +++ b/libsemanage/man/man5/semanage.conf.5
>>> > @@ -121,6 +121,14 @@ and by default it is set to "false".
>>> > Please note that since this option deletes all HLL files, an
>>> > updated HLL
>>> > compiler will not be able to recompile the original HLL file into
>>> > CIL.
>>> > In order to compile the original HLL file into CIL, the same HLL
>>> > file will
>>> > need to be reinstalled.
>>> >
>>> > +.TP
>>> > +.B retain-tmp
>>> > +When set to "true", tmp directories (the sandbox at
>>> > \fBstore-root/\fR[\fIpolicy-store\fR]\fB/tmp \fRand/or the final
>>> > policy at
>>> > \fBstore-root/final/\fR[\fIpolicy-store\fR]) will be retained after
>>> > compilation to allow debugging of any build errors. Note that on a
>>> > successful build the sandbox becomes
>>> > \fBstore-root/\fR[\fIpolicy-store\fR]\fB/active\fR.
>>> > +.br
>>> > +The
>>> > +.B retain-tmp
>>> > +option can be set to either "true" or "false" and by default it is
>>> > set to
>>> > "false".
>>> > +
>>> > .SH "SEE ALSO"
>>> > .TP
>>> > semanage(8)
>>> > diff --git a/libsemanage/src/conf-parse.y b/libsemanage/src/conf-
>>> > parse.y
>>> > index b527e893..f098e55d 100644
>>> > --- a/libsemanage/src/conf-parse.y
>>> > +++ b/libsemanage/src/conf-parse.y
>>> > @@ -61,7 +61,7 @@ static int parse_errors;
>>> >
>>> > %token MODULE_STORE VERSION EXPAND_CHECK FILE_MODE SAVE_PREVIOUS
>>> > SAVE_LINKED TARGET_PLATFORM COMPILER_DIR IGNORE_MODULE_CACHE
>>> > STORE_ROOT
>>> > %token LOAD_POLICY_START SETFILES_START SEFCONTEXT_COMPILE_START
>>> > DISABLE_GENHOMEDIRCON HANDLE_UNKNOWN USEPASSWD IGNOREDIRS
>>> > -%token BZIP_BLOCKSIZE BZIP_SMALL REMOVE_HLL
>>> > +%token BZIP_BLOCKSIZE BZIP_SMALL REMOVE_HLL RETAIN_TMP
>>> > %token VERIFY_MOD_START VERIFY_LINKED_START VERIFY_KERNEL_START
>>> > BLOCK_END
>>> > %token PROG_PATH PROG_ARGS
>>> > %token <s> ARG
>>> > @@ -95,6 +95,7 @@ single_opt: module_store
>>> > | bzip_blocksize
>>> > | bzip_small
>>> > | remove_hll
>>> > + | retain_tmp
>>> > ;
>>> >
>>> > module_store: MODULE_STORE '=' ARG {
>>> > @@ -268,6 +269,17 @@ remove_hll: REMOVE_HLL'=' ARG {
>>> > free($3);
>>> > }
>>> >
>>> > +retain_tmp: RETAIN_TMP'=' ARG {
>>> > + if (strcasecmp($3, "false") == 0) {
>>> > + current_conf->retain_tmp = 0;
>>> > + } else if (strcasecmp($3, "true") == 0) {
>>> > + current_conf->retain_tmp = 1;
>>> > + } else {
>>> > + yyerror("retain-tmp can only be 'true' or
>>> > 'false'");
>>> > + }
>>> > + free($3);
>>> > +}
>>> > +
>>> > command_block:
>>> > command_start external_opts BLOCK_END {
>>> > if (new_external->path == NULL) {
>>> > @@ -352,6 +364,7 @@ static int semanage_conf_init(semanage_conf_t *
>>> > conf)
>>> > conf->bzip_small = 0;
>>> > conf->ignore_module_cache = 0;
>>> > conf->remove_hll = 0;
>>> > + conf->retain_tmp = 0;
>>> >
>>> > conf->save_previous = 0;
>>> > conf->save_linked = 0;
>>> > diff --git a/libsemanage/src/conf-scan.l b/libsemanage/src/conf-
>>> > scan.l
>>> > index 607bbf0b..e26c3494 100644
>>> > --- a/libsemanage/src/conf-scan.l
>>> > +++ b/libsemanage/src/conf-scan.l
>>> > @@ -54,6 +54,7 @@ handle-unknown return HANDLE_UNKNOWN;
>>> > bzip-blocksize return BZIP_BLOCKSIZE;
>>> > bzip-small return BZIP_SMALL;
>>> > remove-hll return REMOVE_HLL;
>>> > +retain-tmp return RETAIN_TMP;
>>> > "[load_policy]" return LOAD_POLICY_START;
>>> > "[setfiles]" return SETFILES_START;
>>> > "[sefcontext_compile]" return SEFCONTEXT_COMPILE_START;
>>> > diff --git a/libsemanage/src/direct_api.c
>>> > b/libsemanage/src/direct_api.c
>>> > index a455612f..5d2a443c 100644
>>> > --- a/libsemanage/src/direct_api.c
>>> > +++ b/libsemanage/src/direct_api.c
>>> > @@ -326,7 +326,10 @@ static void
>>> > semanage_direct_destroy(semanage_handle_t *
>>> > sh
>>> > static int semanage_direct_disconnect(semanage_handle_t * sh)
>>> > {
>>> > /* destroy transaction */
>>> > - if (sh->is_in_transaction) {
>>> > + if (sh->is_in_transaction)
>>> > + semanage_release_trans_lock(sh);
>>> >
>>> >
>>> > Is it safe to release the trans lock before deleting the sandbox?
>>> >
>>> > +
>>> > + if (!sh->conf->retain_tmp && sh->is_in_transaction) {
>>> > /* destroy sandbox */
>>> > if (semanage_remove_directory
>>> > (semanage_path(SEMANAGE_TMP,
>>> > SEMANAGE_TOPLEVEL)) < 0) {
>>> > @@ -342,7 +345,6 @@ static int
>>> > semanage_direct_disconnect(semanage_handle_t
>>> > * sh)
>>> > SEMANAGE_FINAL_TOPL
>>> > EVEL));
>>> > return -1;
>>> > }
>>> > - semanage_release_trans_lock(sh);
>>> > }
>>> >
>>> > /* Release object databases: local modifications */
>>> > @@ -1639,13 +1641,14 @@ cleanup:
>>> >
>>> > 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));
>>> > + /* Delete sandbox unless requested by semanage.conf */
>>> > + if (!sh->conf->retain_tmp) {
>>> > + semanage_remove_directory(semanage_path
>>> > + (SEMANAGE_TMP,
>>> > SEMANAGE_TOPLEVEL));
>>> > + semanage_remove_directory(semanage_final_path
>>> > + (SEMANAGE_FINAL_TMP,
>>> > + SEMANAGE_FINAL_TOPLEVEL)
>>> > );
>>> > + }
>>> > umask(mask);
>>> >
>>> > return retval;
>>> > diff --git a/libsemanage/src/semanage_conf.h
>>> > b/libsemanage/src/semanage_conf.h
>>> > index c99ac8c7..f7bba754 100644
>>> > --- a/libsemanage/src/semanage_conf.h
>>> > +++ b/libsemanage/src/semanage_conf.h
>>> > @@ -46,6 +46,7 @@ typedef struct semanage_conf {
>>> > int bzip_blocksize;
>>> > int bzip_small;
>>> > int remove_hll;
>>> > + int retain_tmp;
>>> > int ignore_module_cache;
>>> > char *ignoredirs; /* ";" separated of list for
>>> > genhomedircon
>>> > to ignore */
>>> > struct external_prog *load_policy;
>>> > --
>>> > 2.14.3
>>> >
>>> >
>>> >
>>>
>>>
>>>
>
>
>
> --
> Respectfully,
>
> William C Roberts
--
Respectfully,
William C Roberts
