findutils-selinux.patch critique (was Re: [patch] Recent findutils test suite failures)

[Tim Waugh]

Forwarding this to a wider SELinux-knowledgeable audience.  James had
asked me what patches we apply to findutils, so I mentioned SELinux
and pointed him to the patch for an initial review.

Tim.
*/

----- Forwarded message from James Youngman <jay at gnu.org> -----

Date: Fri, 25 Nov 2005 08:42:56 +0000
From: James Youngman <jay at gnu.org>
To: Tim Waugh <twaugh at redhat.com>
Cc: bug-findutils at gnu.org
Subject: Re: [patch] Recent findutils test suite failures

On Thu, Nov 24, 2005 at 11:12:19PM +0000, Tim Waugh wrote:

> 2. add support for SELinux.
> 
> At the moment, it isn't autoconf-ed up, but it would be great if you
> wanted to take a look.  SELinux is on by default in Fedora Core.
> 
> http://cvs.fedora.redhat.com/viewcvs/devel/findutils/findutils-selinux.patch?only_with_tag=HEAD&view=markup

I'd certainly consider it.   However, I have some comments/questions.

1. I notice that SELinux-enablement creates two new tests, "-context"
   and "--context".  I would eliminate "--context", leaving only
   "-context".  Find predicates aren't GNU-style long options.  Find
   is confusing enough with options as well as predicates, and so I
   definitely don't want to further blur that line.

2. I see that if you use "find -context" on an SELinux-enabled version
   of find on a system where SELinux is not enabled, find will
   immediately perform a fatal exit with an explanatory message.  I
   won't say that that is definitely the wrong behaviour, but why did
   you choose not to have -context simply always return false (and
   perhaps issue one warning message)?

3. Security contexts are long multi-part strings.  Matching with glob
   patterns is a reasonable approach.  Nevertheless, does the use of
   glob patterns to match them reflect practice on other systems?

4. I notice that your patch slips in a definition for FNM_CASEFOLD.
   That looks wrong to me.  gl_FUNC_FNMATCH_GNU should call
   AC_GNU_SOURCE to ensure that <fnmatch.h> #defines FNM_CASEFOLD for
   you.  This should be called by gl_INIT.  Is this (a) not working
   correctly on some of your build systems, (b) not working on any of
   your build systems, implying a findutils bug, or (c) there for
   'historical reasons'?

5. Findutils should probably default to compiling with SELinux support
   on systems where <selinux/selinux.h> exists or --with-selinux is
   specifified to configure, and compile without such support
   otherwise.   

6. Is there a reason you turn off Automake gnits rules checking in the
   SELinux patch?

7. The configure magic for selinux can be fully generic and
   self-contained I think: we could just have an selinux.m4 file.
   That would also make it much easier to reuse it.

8. Are the DejaGnu test cases for the SELinux patch in another patch
   file somewhere else?  Perhaps it's in the same patch file as the
   update to the Texinfo documentation. :) 

Regards,
James.

----- End forwarded message -----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.fedoraproject.org/pipermail/selinux/attachments/20051125/922625f8/attachment.bin