On Wed, 2011-11-02 at 05:49 +0400, Dmitry V. Levin wrote:
On Tue, Nov 01, 2011 at 08:37:40PM +0100, Tomas Mraz wrote:
[...]
> > I've rebased these changes on top of just released 1.1.5 and pushed to
> >
http://git.altlinux.org/people/ldv/packages/?p=linux-pam.git;a=shortlog;h...
> >
> > Dmitry V. Levin (4):
> > pam_selinux.c: reindent
> > pam_selinux.c: rewrite using pam_get_data/pam_set_data
> > pam_selinux.c: add "restore" option
> > pam_selinux.8.xml: update
> >
> > modules/pam_selinux/pam_selinux.8.xml | 111 +++--
> > modules/pam_selinux/pam_selinux.c | 1076
++++++++++++++++++---------------
> > 2 files changed, 656 insertions(+), 531 deletions(-)
> >
> > This code is already in production for quite a long time, so I suppose
> > it's stable enough and worth your time to have a look at it, finally.
>
> A few comments - I am not too fond of the reindentation commit. Although
> the indentation in pam_selinux.c is ugly and mixing styles I do not
> think this brings us much, but it surely makes things harder to review
> or backport patches into older versions, etc.
I am also not too fond of reindentation, but I had a poor choice. The
indentation in pam_selinux.c is indeed very ugly, and the next commit
changes quite a lot of code anyway: "reindent" commit does
353 insertions and 339 deletions, and "rewrite" commit does
328 insertions and 257 deletions. There are several functions affected by
re-indentation but not affected by later rewrite (send_audit_message,
send_text, query_response, manual_context, mls_range_allowed,
config_context and context_from_env). It is technically possible to
revert that part of the reindentation commit, but this would mean that the
whole indentation will remain ugly and mixing styles. Also, it is not too
late to adjust the indentation style itself, it is not strictly necessary
to choose the style used in larger part of pam_selinux.c code.
I'd really want
to see the opinion of Thorsten or Steve. I am OK with
the reindentation if they are OK with it as well.
> As for the rewrite using pam_get_data/pam_set_data commit
I'd prefer
> using different name for the module_context_t struct - using the word
> context is confusing here with the SELinux contexts. Perhaps
> module_data_t or something else similar would be less confusing.
"module_data_t" is OK for me, but it will cause two additional renames:
"free_context" -> "free_module_data", and "ctx" ->
"data" ("module_data"
would be too long, and "data" doesn't seem to be ambiguous).
Yes,
this rename would be fine.
> For the doc changes:
>
> > - In a nutshell, pam_selinux sets up the default security context for the
> > - next execed shell.
> > + pam_selinux is a PAM module that defines the default SELinux security
> > + context for the next executed process.
> I'd still use 'sets up' instead of 'defines'. Because when
> use_current_range or env_params are used the context is defined
> (partially) elsewhere and pam_selinux just sets the exec context and so
> on.
> The description for restore option should be worded like this:
> "In open_session part of the module, temporarily restore the contexts as
> they were before the previous call of the module. Another call of the
> module without the restore option will set up the new contexts again."
OK for me, but I would change s/contexts/security contexts/ to match
the rest of documentation.
Fine.
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb