Re: [POLICYREP] [RFC/PATCH 1/3] policy package class

[Joshua Brindle <method@manicmethod.com>]

Karl MacMillan wrote:
> On Tue, 2007-07-17 at 11:03 -0400, method@manicmethod.com wrote:
>   
>> plain text document attachment (00-policy_package-header.patch)
>> --
>>
>> --- a/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
>> +++ b/libpolicyrep/include/policyrep/policy_package.hpp	(revision 0)
>> @@ -0,0 +1,38 @@
>> +/* Author: Joshua Brindle <method@manicmethod.com> */
>> +
>> +#ifndef __policy_package_hpp__
>> +#define __policy_package_hpp__
>> +
>> +
>> +#include <policyrep/policy.hpp>
>> +
>> +namespace policyrep
>> +{
>> +	struct PolicyPackageImpl;
>> +	class PolicyPackage
>> +	{
>> +	public:
>> +		PolicyPackage();
>> +		PolicyPackage(std::string filename);
>> +		~PolicyPackage();
>> +
>>     
>
> Destructors should always be virtual - otherwise subclassing is
> difficult.
>   

I don't think there is any reason to ever subclass a policy package. Its 
different from the ast components.
>   
>> +		const Module get_policy_module() const;
>> +
>> +		const std::string get_file_contexts() const;
>> +		const std::string get_seusers() const;
>> +		const std::string get_user_extra() const;
>> +		const std::string get_netfilter_contexts() const;
>> +
>>     
>
> why not const std::string&? I also think we should always default to
> virtual.
>
>   
because this won't be subclassed I don't think these should be virtual.

>> +	protected:
>> +		void init();
>> +		int read_package(std::string filename);	
>> +
>> +	private:
>> +		PolicyPackageImpl* impl;
>>     
>
> We need to make a decision on whether this is going to be protected or
> private and stick with that. I've done protected so far - if you want to
> change it please say why and send a patch changing all existing classes.
>
>   

Same argument. If this isn't subclassed it shouldn't be protected, other 
classes are different that way.
>> +	};
>> +
>> +	typedef boost::shared_ptr<PolicyPackage> PolicyPackagePtr;
>> +
>>     
>
> This may not be needed here - it is currently only definitely used for
> objects that are part of the ast.
>
>   

I like it, it will enable users to have several pointers to the package 
and have them dealt with correctly. Do you disagree?


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.