On Mon, 2010-02-01 at 17:36 +0100, Guido Trentalancia wrote: > Excellent Stephen, very timely ! > > I am now going to make the necessary amendments and then I will post a proposed final patch that you can eventually review again or otherwise just add an Acked-by or Reviewed-by line and I will forward it to the kernel mailing list. > > Of course comments from other list members are always welcome. > > A few notes on your comments... > > > Avoid adding extern declarations to .c files - they are > > discouraged. The current code isn't pristine in this > > regard, but let's not make it worse. > > You are right, I did check with checkpatch.pl, but because selinuxfs.c hasn't got its own header file, I wasn't sure on how to deal with the warning. Shall I create an header file specifically for it or otherwise try to reuse other global header files ? In this case, you would just add a function prototype for a new security_mls_enabled() function to include/security.h, implement it in ss/services.c, and call it from selinuxfs.c. > > I wouldn't take these functions to their own .c file, and > > if you do so, then "inline" no longer makes sense. But > > why did you do so? It will add overhead to make them > > out-of-line with no obvious benefit. > > I was getting compilation errors. Perhaps you can give it a try. That's a problem with your patch, not with the existing code. Keep the functions as static inline functions in their .h files. > > This seems inconsistent with the other functions - here > > you pass the policydb as an argument rather than just > > using the active policydb. If that were truly necessary, > > I'd argue for passing the policydb to all of the > > functions for consistency. But I think in fact that we > > can drop this !mls_enabled check altogether, given that > > the context is fully initialized by context_init() and > > thus the ebitmap_destroy() calls should be safe > > regardless of whether or not MLS is enabled. > > The policydb being passed to the modified functions is not necessarily the same as the active policy. Otherwise there was no point of making the modifications. To be more precise, consider the code-paths from security_load_policy() when you need to destroy the old policy and the old sidtab which are no longer active ! > So, shall I remove completely the check ? That would make > mls_context_destroy() execute even for standard policies... Yes, that should cause no harm. > > Same deal - don't take these to their own .c file without > > justification. > > As already explained, I moved those functions to a separate .c file > because there were compilation error (gcc 4.4.1). The current code compiles fine, so that would be a problem with your patch. Don't move the functions to a separate .c file. static inline is fine. If you have a compilation problem, post the details. > > Is that supposed to be "n" aka newline rather than just > > "n"? > > Yes, of course "n". It's weird because if you look at the attached file, everything is normal ! Something went wrong with the "cut-n-paste" from gedit to Mozilla, I shall probably file a bug report !... Read Documentation/email-clients.txt, particularly about thunderbird if you are using it. -- Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with the words "unsubscribe selinux" without quotes as the message.
