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

[Joshua Brindle <method@manicmethod.com>]

Karl MacMillan wrote:
> On Tue, 2007-07-17 at 12:23 -0400, Joshua Brindle wrote:
>   
>> 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.
>>     
>
> Except that if the destructor is not virtual you cannot effectively
> subclass without breaking binary compatibility. Also - in this case
> there is no real downside to virtual by default, so why not do it?
>
>   

Fine.

>>>   
>>>       
>>>> +		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.
>>
>>     
>
> See above. What about the return type, though?
>
>   

I just don't see it as advantageous, since functions aren't part of the 
mangled class struct the ABI won't change if they are added later as 
virtuals (assuming there is no other virtuals below that will change 
alignment and everything else is private). Also, if we determine that, 
e.g. seusers have no business in modules we can remove the functions 
without destroying the rest of the function alignments which will only 
affect the API not the ABI, for this reason I'd prefer to avoid virtuals 
unless there is a real possibility of the class being subclassed.

On the std::string&, I've seen arguments against making std::strings 
pointers because then you don't need to deal with initialization and the 
string data itself is on the heap anyway, whats the advantage to making 
these pointers?
>>>> +	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.
>>     



--
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.