policy package implementation with xar
cover policy package implementation with xar
This uses xar <http://code.google.com/p/xar/> to implement policy packages. This brings in a fair number of dependancies, unfortunately, but provides a useful featureset in exchange. This includes transparent compression of files in the package, signature support and so on.
Currently it uses the filename to determine the kind of file (eg., file_context file vs. policy module) which is non-ideal, I think it might be better to use xar attributes in the ToC to specify the file but that means we'd have to implement our own packaging functions and could not use the xar command line utility to create packages. Since we don't currently do anything special like that there is no package_write functionality (or set operators for the implimentation).
I also have concerns about using the module name property as that should be abstract to this code, instead using a xar subdocument could allow us to define the 'name' of the policy as a policy package attribute instead of putting it in the module. This is completely different from how the current code works but I feel like the name should be associated with the policy package rather than the module.
Comments welcome. This is primarilly an RFC to see if this is how we want to handle policy packages, though it should be mergable in its current state if everyone agrees this is the ideal implementation.
FWIW I also looked for more 'lightweight' archival systems and found that no libraries exist for tar, ar or cpio. A quick search of yum only shows one archive library and it is zip format.
Signature · 4 lines
-- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
PATCH 1/3 policy package class
--
libpolicyrep/include/policyrep/policy_package.hpp +38 -0
@@ -0,0 +1,38 @@ +/* Author: Joshua Brindle <[email protected]> */ + +#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(); + + 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; + + protected: + void init(); + int read_package(std::string filename); + + private: + PolicyPackageImpl* impl; + }; + + typedef boost::shared_ptr<PolicyPackage> PolicyPackagePtr; + +} // namespace policyrep + +#endif
Signature · 4 lines
-- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 1/3] policy package class
Quoted 14 lines · On Tue, 2007-07-17 at 11:03 -0400, [email protected] wrote:
plain text document attachment (00-policy_package-header.patch) --
libpolicyrep/include/policyrep/policy_package.hpp +8
-0@@ -0,0 +1,38 @@ +/* Author: Joshua Brindle <[email protected]> */ + +#ifndef __policy_package_hpp__ +#define __policy_package_hpp__ + + +#include <policyrep/policy.hpp> +
libpolicyrep/include/policyrep/policy_package.hpp line 18
Quoted 10 lines
libpolicyrep/include/policyrep/policy_package.hpp +10
-0+namespace policyrep +{ + struct PolicyPackageImpl; + class PolicyPackage + { + public: + PolicyPackage(); + PolicyPackage(std::string filename); + ~PolicyPackage(); +
Destructors should always be virtual - otherwise subclassing is difficult.
libpolicyrep/include/policyrep/policy_package.hpp line 25
Quoted 7 lines
libpolicyrep/include/policyrep/policy_package.hpp +7
-0+ 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.
libpolicyrep/include/policyrep/policy_package.hpp line 31
Quoted 6 lines
libpolicyrep/include/policyrep/policy_package.hpp +6
-0+ 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.
libpolicyrep/include/policyrep/policy_package.hpp line 35
Quoted 4 lines
libpolicyrep/include/policyrep/policy_package.hpp +4
-0+ }; + + 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.
Karl
libpolicyrep/include/policyrep/policy_package.hpp line 38
Quoted 4 lines
libpolicyrep/include/policyrep/policy_package.hpp +3
-0+} // namespace policyrep + +#endif
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 1/3] policy package class
Quoted 22 lines · Karl MacMillan
Quoted 20 lines · On Tue, 2007-07-17 at 11:03 -0400, [email protected] wrote:
plain text document attachment (00-policy_package-header.patch) --
libpolicyrep/include/policyrep/policy_package.hpp +14
-0@@ -0,0 +1,38 @@ +/* Author: Joshua Brindle <[email protected]> */ + +#ifndef __policy_package_hpp__ +#define __policy_package_hpp__ + + +#include <policyrep/policy.hpp> + +namespace policyrep +{ + struct PolicyPackageImpl; + class PolicyPackage + { + public:
Quoted 9 lines
libpolicyrep/include/policyrep/policy_package.hpp line 18
Quoted 5 lines
libpolicyrep/include/policyrep/policy_package.hpp +4
-0+ 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.
Quoted 14 lines
libpolicyrep/include/policyrep/policy_package.hpp line 25
Quoted 9 lines
libpolicyrep/include/policyrep/policy_package.hpp +7
-0+ 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.
Quoted 13 lines
libpolicyrep/include/policyrep/policy_package.hpp line 31
Quoted 7 lines
libpolicyrep/include/policyrep/policy_package.hpp +6
-0+ 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.
Quoted 10 lines
libpolicyrep/include/policyrep/policy_package.hpp line 35
Quoted 5 lines
libpolicyrep/include/policyrep/policy_package.hpp +4
-0+ }; + + 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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 1/3] policy package class
Quoted 33 lines · Joshua Brindle
Quoted 22 lines · Karl MacMillan
Quoted 20 lines · On Tue, 2007-07-17 at 11:03 -0400, [email protected] wrote:
plain text document attachment (00-policy_package-header.patch) --
libpolicyrep/include/policyrep/policy_package.hpp +14
-0@@ -0,0 +1,38 @@ +/* Author: Joshua Brindle <[email protected]> */ + +#ifndef __policy_package_hpp__ +#define __policy_package_hpp__ + + +#include <policyrep/policy.hpp> + +namespace policyrep +{ + struct PolicyPackageImpl; + class PolicyPackage + { + public:Quoted 9 lines
libpolicyrep/include/policyrep/policy_package.hpp line 18
Quoted 5 lines
libpolicyrep/include/policyrep/policy_package.hpp +4
-0+ PolicyPackage(); + PolicyPackage(std::string filename); + ~PolicyPackage(); +Destructors should always be virtual - otherwise subclassing is difficult.
Quoted 2 lines
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?
Quoted 14 lines
Quoted 14 lines
libpolicyrep/include/policyrep/policy_package.hpp line 25
Quoted 9 lines
libpolicyrep/include/policyrep/policy_package.hpp +7
-0+ 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.
Quoted 2 lines
because this won't be subclassed I don't think these should be virtual.
See above. What about the return type, though?
Quoted 27 lines
Quoted 13 lines
libpolicyrep/include/policyrep/policy_package.hpp line 31
Quoted 7 lines
libpolicyrep/include/policyrep/policy_package.hpp +6
-0+ 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.
Quoted 10 lines
libpolicyrep/include/policyrep/policy_package.hpp line 35
Quoted 5 lines
libpolicyrep/include/policyrep/policy_package.hpp +4
-0+ }; + + 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.
Quoted 3 lines
I like it, it will enable users to have several pointers to the package and have them dealt with correctly. Do you disagree?
No - I was just noting that it wasn't strictly necessary. If you find it useful I'm fine with it.
Karl
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 1/3] policy package class
Quoted 37 lines · Karl MacMillan
Quoted 35 lines · Joshua Brindle
Quoted 33 lines · Karl MacMillan
Quoted 16 lines · On Tue, 2007-07-17 at 11:03 -0400, [email protected] wrote:
plain text document attachment (00-policy_package-header.patch) --
libpolicyrep/include/policyrep/policy_package.hpp +8
-0@@ -0,0 +1,38 @@ +/* Author: Joshua Brindle <[email protected]> */ + +#ifndef __policy_package_hpp__ +#define __policy_package_hpp__ + + +#include <policyrep/policy.hpp> +libpolicyrep/include/policyrep/policy_package.hpp line 18
Quoted 10 lines
libpolicyrep/include/policyrep/policy_package.hpp +10
-0+namespace policyrep +{ + struct PolicyPackageImpl; + class PolicyPackage + { + public: + PolicyPackage(); + PolicyPackage(std::string filename); + ~PolicyPackage(); +Destructors should always be virtual - otherwise subclassing is difficult.
Quoted 8 lines
Quoted 2 lines
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.
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
libpolicyrep/include/policyrep/policy_package.hpp line 25
Quoted 11 lines
libpolicyrep/include/policyrep/policy_package.hpp +7
-0+ 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.
Quoted 7 lines
Quoted 3 lines
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?
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
libpolicyrep/include/policyrep/policy_package.hpp line 31
Quoted 8 lines
libpolicyrep/include/policyrep/policy_package.hpp +6
-0+ 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.
Quoted 2 lines
Quoted 2 lines
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 [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 1/3] policy package class
Quoted 70 lines · Joshua Brindle
Quoted 37 lines · Karl MacMillan
Quoted 35 lines · Joshua Brindle
Quoted 33 lines · Karl MacMillan
Quoted 16 lines · On Tue, 2007-07-17 at 11:03 -0400, [email protected] wrote:
plain text document attachment (00-policy_package-header.patch) --
libpolicyrep/include/policyrep/policy_package.hpp +8
-0@@ -0,0 +1,38 @@ +/* Author: Joshua Brindle <[email protected]> */ + +#ifndef __policy_package_hpp__ +#define __policy_package_hpp__ + + +#include <policyrep/policy.hpp> +libpolicyrep/include/policyrep/policy_package.hpp line 18
Quoted 10 lines
libpolicyrep/include/policyrep/policy_package.hpp +10
-0+namespace policyrep +{ + struct PolicyPackageImpl; + class PolicyPackage + { + public: + PolicyPackage(); + PolicyPackage(std::string filename); + ~PolicyPackage(); +Destructors should always be virtual - otherwise subclassing is difficult.
Quoted 9 lines
Quoted 3 lines
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.
Quoted 20 lines
Quoted 19 lines
Quoted 16 lines
libpolicyrep/include/policyrep/policy_package.hpp line 25
Quoted 11 lines
libpolicyrep/include/policyrep/policy_package.hpp +7
-0+ 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.
Quoted 8 lines
Quoted 3 lines
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).
Not certain what you mean, but you can't convert a function to virtual later and retain ABI.
Quoted 4 lines
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,
Err - of course it will effect the ABI. The runtime behavior will be _vastly_ different. It is, I think, better to break ABI in these cases so that you don't get undefined runtime behavior.
Quoted 3 lines
for this reason I'd prefer to avoid virtuals
unless there is a real possibility of the class being subclassed.
But there is at least some chance - what if I want to create a different kind of package? Or a fake package that is just some files on disk? The whole point is that a class that the potential of subclassing adds real flexibility to the code.
Quoted 4 lines
On the std::string&, I've seen arguments against making
std::stringspointers 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?
Umm - I'm suggesting that you should return references not pointers. It avoids copies and allows the caller to change the strings.
Karl
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
PATCH 2/3 policy package implementation
In reply to [email protected]--
libpolicyrep/src/policy_package.cpp +166 -0
@@ -0,0 +1,166 @@ +/* + * Author : Joshua Brindle <[email protected]> + * + * Copyright (C) 2007 Tresys Technology, llc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + + +extern "C" { +#include <xar/xar.h> +#include <string.h> +} + +#include <policyrep/policy.hpp> +#include <policyrep/policy_package.hpp> + +#include <sstream> +#include <iostream> + +namespace policyrep +{ + + struct PolicyPackageImpl { + Module policy_module; + std::string file_contexts; + std::string seusers; + std::string user_extra; + std::string netfilter_contexts; + }; + + void PolicyPackage::init() { + impl = new PolicyPackageImpl; + } + + int PolicyPackage::read_package(std::string filename) { + xar_t x; + xar_file_t f; + xar_iter_t i; + + i = xar_iter_new(); + if (i == NULL) { + throw "Unable to allocate iterator"; + } + + x = xar_open(filename.c_str(), READ); + if (x == NULL) { + throw "Unable to open policy package"; + } + + for (f = xar_file_first(x, i); f; f = xar_file_next(i)) { + size_t sz; + char *fbuf; + const char *filename; + int32_t ret; + + ret = xar_extract_tobuffersz(x, f, &fbuf, &sz); + if (ret) { + // This can happen if the file is 0 bytes + // or is a symlink, directory, etc. We might want + // to put code here to check those cases and bail + // but for now we just ignore them and continue. + continue; + } + + + // Currently we use the name of the file to decide what kind of file it is + // I am fairly uncomfortable with this but it allows us to use xar to create + // policy packages instead of having our own write_package function that + // assigns attributes to files instead of using names. + ret = xar_prop_get(f, "name", &filename); + if (ret) { + xar_close(x); + throw "Error getting name property of file"; + } + if (strstr(filename, ".mod")) { + // found module, do stuff + } else if (strstr(filename, ".file_contexts")) { + if (!impl->file_contexts.empty()) { + // Found more than one file containing in .file_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple file_contexts files in policy package"; + } + impl->file_contexts = std::string(fbuf); + } else if (strstr(filename, ".seusers")) { + if (!impl->seusers.empty()) { + // Found more than one file containing in .seusers, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple seusers files in policy package"; + } + impl->seusers = std::string(fbuf); + } else if (strstr(filename, ".user_extra")) { + if (!impl->user_extra.empty()) { + // Found more than one file containing in .user_extra, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple user_extra files in policy package"; + } + impl->user_extra = std::string(fbuf); + } else if (strstr(filename, ".netfilter_contexts")) { + if (!impl->netfilter_contexts.empty()) { + // Found more than one file containing in .netfilter_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple netfilter_contexts files in policy package"; + } + impl->netfilter_contexts = std::string(fbuf); + } else { + // unrecognized file, just skip it + continue; + } + + } + + xar_close(x); + + return 0; + } + + PolicyPackage::PolicyPackage() { + init(); + } + + PolicyPackage::PolicyPackage(std::string filename) { + init(); + read_package(filename); + } + + PolicyPackage::~PolicyPackage() { delete impl; } + + const Module PolicyPackage::get_policy_module() const { + return impl->policy_module; + } + + const std::string PolicyPackage::get_file_contexts() const { + return impl->file_contexts; + } + + const std::string PolicyPackage::get_seusers() const { + return impl->seusers; + } + + const std::string PolicyPackage::get_user_extra() const { + return impl->user_extra; + } + + const std::string PolicyPackage::get_netfilter_contexts() const { + return impl->netfilter_contexts; + } + + +} // namespace policyrep
Signature · 4 lines
-- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
Quoted 33 lines · On Tue, 2007-07-17 at 11:03 -0400, [email protected] wrote:
plain text document attachment (01-policy_package.diff) --
libpolicyrep/src/policy_package.cpp +27
-0@@ -0,0 +1,166 @@ +/* + * Author : Joshua Brindle <[email protected]> + * + * Copyright (C) 2007 Tresys Technology, llc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + + +extern "C" { +#include <xar/xar.h> +#include <string.h> +} + +#include <policyrep/policy.hpp>
libpolicyrep/src/policy_package.cpp line 37
Quoted 10 lines
libpolicyrep/src/policy_package.cpp +10
-0+#include <policyrep/policy_package.hpp> + +#include <sstream> +#include <iostream> + +namespace policyrep +{ + + struct PolicyPackageImpl { + Module policy_module;
Are you sure you want this by value?
libpolicyrep/src/policy_package.cpp line 47
Quoted 10 lines
libpolicyrep/src/policy_package.cpp +10
-0+ std::string file_contexts; + std::string seusers; + std::string user_extra; + std::string netfilter_contexts; + }; + + void PolicyPackage::init() { + impl = new PolicyPackageImpl; + } +
I prefer to order the .cpp file in the same order functions are declared in the class. I think it makes it easier to find the function you want.
libpolicyrep/src/policy_package.cpp line 55
Quoted 8 lines
libpolicyrep/src/policy_package.cpp +8
-0+ int PolicyPackage::read_package(std::string filename) { + xar_t x; + xar_file_t f; + xar_iter_t i; + + i = xar_iter_new(); + if (i == NULL) { + throw "Unable to allocate iterator";
Ick - no string exceptions please. Either use an exception in stdexcept or define a new one (std::bad_alloc is appropriate here).
Quoted 17 lines
libpolicyrep/src/policy_package.cpp +17
-0+ } + + x = xar_open(filename.c_str(), READ); + if (x == NULL) { + throw "Unable to open policy package"; + } + + for (f = xar_file_first(x, i); f; f = xar_file_next(i)) { + size_t sz; + char *fbuf; + const char *filename; + int32_t ret; + + ret = xar_extract_tobuffersz(x, f, &fbuf, &sz); + if (ret) { + // This can happen if the file is 0 bytes + // or is a symlink, directory, etc. We might want
libpolicyrep/src/policy_package.cpp line 82
Quoted 10 lines
libpolicyrep/src/policy_package.cpp +10
-0+ // to put code here to check those cases and bail + // but for now we just ignore them and continue. + continue; + } + + + // Currently we use the name of the file to decide what kind of file it is + // I am fairly uncomfortable with this but it allows us to use xar to create + // policy packages instead of having our own write_package function that + // assigns attributes to files instead of using names.
I don't like this either - what is the alternative?
libpolicyrep/src/policy_package.cpp line 86
Quoted 4 lines
libpolicyrep/src/policy_package.cpp +4
-0+ ret = xar_prop_get(f, "name", &filename); + if (ret) { + xar_close(x); + throw "Error getting name property of file";
Please define an exception.
libpolicyrep/src/policy_package.cpp line 89
Quoted 3 lines
libpolicyrep/src/policy_package.cpp +3
-0+ } + if (strstr(filename, ".mod")) { + // found module, do stuff
Err - "do stuff"?
Quoted 6 lines
libpolicyrep/src/policy_package.cpp +6
-0+ } else if (strstr(filename, ".file_contexts")) { + if (!impl->file_contexts.empty()) { + // Found more than one file containing in .file_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple file_contexts files in policy package";
libpolicyrep/src/policy_package.cpp line 105
Quoted 10 lines
libpolicyrep/src/policy_package.cpp +10
-0+ } + impl->file_contexts = std::string(fbuf); + } else if (strstr(filename, ".seusers")) { + if (!impl->seusers.empty()) { + // Found more than one file containing in .seusers, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple seusers files in policy package"; + } + impl->seusers = std::string(fbuf);
As we discussed privately - these copies are not great. Perhaps just use raw char*.
Quoted 32 lines
libpolicyrep/src/policy_package.cpp +32
-0+ } else if (strstr(filename, ".user_extra")) { + if (!impl->user_extra.empty()) { + // Found more than one file containing in .user_extra, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple user_extra files in policy package"; + } + impl->user_extra = std::string(fbuf); + } else if (strstr(filename, ".netfilter_contexts")) { + if (!impl->netfilter_contexts.empty()) { + // Found more than one file containing in .netfilter_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple netfilter_contexts files in policy package"; + } + impl->netfilter_contexts = std::string(fbuf); + } else { + // unrecognized file, just skip it + continue; + } + + } + + xar_close(x); + + return 0; + } + + PolicyPackage::PolicyPackage() { + init(); + } +
libpolicyrep/src/policy_package.cpp line 147
Quoted 10 lines
libpolicyrep/src/policy_package.cpp +10
-0+ PolicyPackage::PolicyPackage(std::string filename) { + init(); + read_package(filename); + } + + PolicyPackage::~PolicyPackage() { delete impl; } + + const Module PolicyPackage::get_policy_module() const { + return impl->policy_module; + }
This should definitely not be by value. It should also likely not be const - this is how the user will retrieve the module after reading to use, correct? Then it shouldn't be const (they might want to much with it), should be a reference, and shouldn't be const for the class (as allowing users to change the module changes the object).
Quoted 7 lines
libpolicyrep/src/policy_package.cpp +7
-0+ + const std::string PolicyPackage::get_file_contexts() const { + return impl->file_contexts; + } + + const std::string PolicyPackage::get_seusers() const { + return impl->seusers;
libpolicyrep/src/policy_package.cpp line 164
Quoted 10 lines
libpolicyrep/src/policy_package.cpp +10
-0+ } + + const std::string PolicyPackage::get_user_extra() const { + return impl->user_extra; + } + + const std::string PolicyPackage::get_netfilter_contexts() const { + return impl->netfilter_contexts; + } +
Ditto.
libpolicyrep/src/policy_package.cpp line 166
Quoted 3 lines
libpolicyrep/src/policy_package.cpp +2
-0+ +} // namespace policyrep
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
Quoted 12 lines · Karl MacMillan
libpolicyrep/src/policy_package.cpp line 37
Quoted 8 lines
libpolicyrep/src/policy_package.cpp +6
-0+ +namespace policyrep +{ + + struct PolicyPackageImpl { + Module policy_module;Are you sure you want this by value?
oops, yes, fixed.
Quoted 6 lines
Quoted 6 lines
libpolicyrep/src/policy_package.cpp +6
-0+ std::string file_contexts; + std::string seusers; + std::string user_extra; + std::string netfilter_contexts; + }; +
Quoted 10 lines
libpolicyrep/src/policy_package.cpp line 47
Quoted 5 lines
libpolicyrep/src/policy_package.cpp +4
-0+ void PolicyPackage::init() { + impl = new PolicyPackageImpl; + } +I prefer to order the .cpp file in the same order functions are declared in the class. I think it makes it easier to find the function you want.
I reordered them, I was still use to having to order based on usage (eg., read_package first since its called from one of the constructors.. silly C habits :\)
Quoted 4 lines
Quoted 4 lines
libpolicyrep/src/policy_package.cpp +4
-0+ int PolicyPackage::read_package(std::string filename) { + xar_t x; + xar_file_t f; + xar_iter_t i;
Quoted 10 lines
libpolicyrep/src/policy_package.cpp line 55
Quoted 5 lines
libpolicyrep/src/policy_package.cpp +4
-0+ + i = xar_iter_new(); + if (i == NULL) { + throw "Unable to allocate iterator";Ick - no string exceptions please. Either use an exception in stdexcept or define a new one (
std::bad_allocis appropriate here).
fixed.
Quoted 22 lines
Quoted 22 lines
libpolicyrep/src/policy_package.cpp +22
-0+ } + + x = xar_open(filename.c_str(), READ); + if (x == NULL) { + throw "Unable to open policy package"; + } + + for (f = xar_file_first(x, i); f; f = xar_file_next(i)) { + size_t sz; + char *fbuf; + const char *filename; + int32_t ret; + + ret = xar_extract_tobuffersz(x, f, &fbuf, &sz); + if (ret) { + // This can happen if the file is 0 bytes + // or is a symlink, directory, etc. We might want + // to put code here to check those cases and bail + // but for now we just ignore them and continue. + continue; + } +
Quoted 10 lines
libpolicyrep/src/policy_package.cpp line 82
Quoted 6 lines
libpolicyrep/src/policy_package.cpp +5
-0+ + // Currently we use the name of the file to decide what kind of file it is + // I am fairly uncomfortable with this but it allows us to use xar to create + // policy packages instead of having our own write_package function that + // assigns attributes to files instead of using names.I don't like this either - what is the alternative?
Ok, xar has an XML ToC for the file. It also provides an API to set arbitrary attributes for each file, so I can do something like selinuxfiletype = netfilter_contexts. This means that we would not be able to use the command line xar to create packages (I was trying to make this as simple as possible). If we move to using attributes we'd have to have a custom packager again (ala semodule_package).
A custom packager has advantages, we can basically ensure that the package is sane (doesn't have directory trees or multiple <something>.netfilter_contexts files, etc. We can also add new properties such as the policy name to the xar XML subdocument.
Quoted 9 lines
libpolicyrep/src/policy_package.cpp line 86
Quoted 5 lines
libpolicyrep/src/policy_package.cpp +4
-0+ ret = xar_prop_get(f, "name", &filename); + if (ret) { + xar_close(x); + throw "Error getting name property of file";Please define an exception.
Why?
Quoted 8 lines
libpolicyrep/src/policy_package.cpp line 89
Quoted 4 lines
libpolicyrep/src/policy_package.cpp +3
-0+ } + if (strstr(filename, ".mod")) { + // found module, do stuffErr - "do stuff"?
There is no module_read function right now so thats just a placeholder.
Quoted 12 lines
Quoted 12 lines
libpolicyrep/src/policy_package.cpp +12
-0+ } else if (strstr(filename, ".file_contexts")) { + if (!impl->file_contexts.empty()) { + // Found more than one file containing in .file_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple file_contexts files in policy package"; + } + impl->file_contexts = std::string(fbuf); + } else if (strstr(filename, ".seusers")) { + if (!impl->seusers.empty()) { + // Found more than one file containing in .seusers, + // don't know how to handle that so bail for now
Quoted 10 lines
libpolicyrep/src/policy_package.cpp line 105
Quoted 5 lines
libpolicyrep/src/policy_package.cpp +4
-0+ xar_close(x); + throw "Multiple seusers files in policy package"; + } + impl->seusers = std::string(fbuf);As we discussed privately - these copies are not great. Perhaps just use raw char*.
Ok, I don't know if this code is ever going to do anything with those and was trying to keep it in a format that would be more accessible if we needed to. I can change to char * though and see if anything develops that makes std::string make more sense.
Quoted 41 lines
Quoted 41 lines
libpolicyrep/src/policy_package.cpp +41
-0+ } else if (strstr(filename, ".user_extra")) { + if (!impl->user_extra.empty()) { + // Found more than one file containing in .user_extra, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple user_extra files in policy package"; + } + impl->user_extra = std::string(fbuf); + } else if (strstr(filename, ".netfilter_contexts")) { + if (!impl->netfilter_contexts.empty()) { + // Found more than one file containing in .netfilter_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple netfilter_contexts files in policy package"; + } + impl->netfilter_contexts = std::string(fbuf); + } else { + // unrecognized file, just skip it + continue; + } + + } + + xar_close(x); + + return 0; + } + + PolicyPackage::PolicyPackage() { + init(); + } + + PolicyPackage::PolicyPackage(std::string filename) { + init(); + read_package(filename); + } + + PolicyPackage::~PolicyPackage() { delete impl; } + + const Module PolicyPackage::get_policy_module() const { + return impl->policy_module;
Quoted 10 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ }This should definitely not be by value. It should also likely not be const - this is how the user will retrieve the module after reading to use, correct? Then it shouldn't be const (they might want to much with it), should be a reference, and shouldn't be const for the class (as allowing users to change the module changes the object).
Fair enough. I was basically thinking users would read a module and make their own copies (eg., copying the data into a merged copy or something) but that isn't really how policyrep works so I'll change all these to be non-const.
Quoted 12 lines
Quoted 12 lines
libpolicyrep/src/policy_package.cpp +12
-0+ + const std::string PolicyPackage::get_file_contexts() const { + return impl->file_contexts; + } + + const std::string PolicyPackage::get_seusers() const { + return impl->seusers; + } + + const std::string PolicyPackage::get_user_extra() const { + return impl->user_extra; + }
Quoted 9 lines
libpolicyrep/src/policy_package.cpp line 164
Quoted 6 lines
libpolicyrep/src/policy_package.cpp +5
-0+ + const std::string PolicyPackage::get_netfilter_contexts() const { + return impl->netfilter_contexts; + } +Ditto.
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
Quoted 1 line · Joshua Brindle
Karl MacMillan wrote:
[...]
Quoted 10 lines
Quoted 9 lines
libpolicyrep/src/policy_package.cpp line 82
Quoted 5 lines
libpolicyrep/src/policy_package.cpp +4
-0+ // Currently we use the name of the file to decide what kind of file it is + // I am fairly uncomfortable with this but it allows us to use xar to create + // policy packages instead of having our own write_package function that + // assigns attributes to files instead of using names.I don't like this either - what is the alternative?
Quoted 7 lines
Ok, xar has an XML ToC for the file. It also provides an API to set arbitrary attributes for each file, so I can do something like selinuxfiletype =
netfilter_contexts. This means that we would not be able to use the command line xar to create packages (I was trying to make this as simple as possible). If we move to using attributes we'd have to have a custom packager again (alasemodule_package).
It would be simpler though, correct? And what about an unpackager?
Quoted 5 lines
A custom packager has advantages, we can basically ensure that the package is sane (doesn't have directory trees or multiple <something>.netfilter_contexts files, etc. We can also add new properties such as the policy name to the xar XML subdocument.
That sounds better than relying on file extension to me.
Quoted 11 lines
Quoted 9 lines
libpolicyrep/src/policy_package.cpp line 86
Quoted 5 lines
libpolicyrep/src/policy_package.cpp +4
-0+ ret = xar_prop_get(f, "name", &filename); + if (ret) { + xar_close(x); + throw "Error getting name property of file";Please define an exception.
Why?
Because none of the standard exception types seemed to fit and string exceptions are not a good idea.
Quoted 10 lines
Quoted 8 lines
libpolicyrep/src/policy_package.cpp line 89
Quoted 4 lines
libpolicyrep/src/policy_package.cpp +3
-0+ } + if (strstr(filename, ".mod")) { + // found module, do stuffErr - "do stuff"?
There is no
module_readfunction right now so thats just a placeholder.
There is if the module is source :)
Quoted 79 lines
Quoted 12 lines
Quoted 12 lines
libpolicyrep/src/policy_package.cpp +12
-0+ } else if (strstr(filename, ".file_contexts")) { + if (!impl->file_contexts.empty()) { + // Found more than one file containing in .file_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple file_contexts files in policy package"; + } + impl->file_contexts = std::string(fbuf); + } else if (strstr(filename, ".seusers")) { + if (!impl->seusers.empty()) { + // Found more than one file containing in .seusers, + // don't know how to handle that so bail for nowQuoted 10 lines
libpolicyrep/src/policy_package.cpp line 105
Quoted 5 lines
libpolicyrep/src/policy_package.cpp +4
-0+ xar_close(x); + throw "Multiple seusers files in policy package"; + } + impl->seusers = std::string(fbuf);As we discussed privately - these copies are not great. Perhaps just use raw char*.
Ok, I don't know if this code is ever going to do anything with those and was trying to keep it in a format that would be more accessible if we needed to. I can change to char * though and see if anything develops that makes
std::stringmake more sense.Quoted 41 lines
Quoted 41 lines
libpolicyrep/src/policy_package.cpp +41
-0+ } else if (strstr(filename, ".user_extra")) { + if (!impl->user_extra.empty()) { + // Found more than one file containing in .user_extra, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple user_extra files in policy package"; + } + impl->user_extra = std::string(fbuf); + } else if (strstr(filename, ".netfilter_contexts")) { + if (!impl->netfilter_contexts.empty()) { + // Found more than one file containing in .netfilter_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple netfilter_contexts files in policy package"; + } + impl->netfilter_contexts = std::string(fbuf); + } else { + // unrecognized file, just skip it + continue; + } + + } + + xar_close(x); + + return 0; + } + + PolicyPackage::PolicyPackage() { + init(); + } + + PolicyPackage::PolicyPackage(std::string filename) { + init(); + read_package(filename); + } + + PolicyPackage::~PolicyPackage() { delete impl; } + + const Module PolicyPackage::get_policy_module() const { + return impl->policy_module;Quoted 10 lines
Review
Quoted 2 lines
(quoted diff) +1
-0+ }This should definitely not be by value. It should also likely not be const - this is how the user will retrieve the module after reading to use, correct? Then it shouldn't be const (they might want to much with it), should be a reference, and shouldn't be const for the class (as allowing users to change the module changes the object).
Quoted 5 lines
Fair enough. I was basically thinking users would read a module and make their own copies (eg., copying the data into a merged copy or something) but that isn't really how policyrep works so I'll change all these to be non-const.
One other note - if the caller is expected to take ownership for the lifetime of the object (which seems possible), then it should return a pointer or a shared_ptr.
Karl
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
Karl MacMillan wrote: On Tue, 2007-07-17 at 12:40 -0400, Joshua Brindle wrote: Karl MacMillan wrote:
Joshua Brindle
NAKRe: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
In reply to Karl MacMillanQuoted 16 lines · Karl MacMillan
Quoted 2 lines · Joshua Brindle
Karl MacMillan wrote:
[...]
Quoted 10 lines
Quoted 10 lines
libpolicyrep/src/policy_package.cpp line 82
Quoted 6 lines
libpolicyrep/src/policy_package.cpp +4
-0+ // Currently we use the name of the file to decide what kind of file it is + // I am fairly uncomfortable with this but it allows us to use xar to create + // policy packages instead of having our own write_package function that + // assigns attributes to files instead of using names.I don't like this either - what is the alternative?
Quoted 6 lines
Quoted 3 lines
Ok, xar has an XML ToC for the file. It also provides an API to set arbitrary attributes for each file, so I can do something like selinuxfiletype =
netfilter_contexts. This means that we would not be able to use the command line xar to create packages (I was trying to make this as simple as possible). If we move to using attributes we'd have to have a custom packager again (alasemodule_package).It would be simpler though, correct? And what about an unpackager?
Simpler in what way? This first patch I was trying to keep it dead simple by not even requiring a custom packager and instead just using xar CLI. If we want to use more sophisticated things in xar (like per-file attributes) we'd need something custom. Even if we just make a subdocument (which is archive-wide in scope) and add some selinux bits to it like policy name that can still be done with the CLI.
xar -xf foo.xar will unpackage it
Quoted 8 lines
Quoted 4 lines
A custom packager has advantages, we can basically ensure that the package is sane (doesn't have directory trees or multiple <something>.netfilter_contexts files, etc. We can also add new properties such as the policy name to the xar XML subdocument.
That sounds better than relying on file extension to me.
agreed.
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
libpolicyrep/src/policy_package.cpp line 86
Quoted 6 lines
libpolicyrep/src/policy_package.cpp +4
-0+ ret = xar_prop_get(f, "name", &filename); + if (ret) { + xar_close(x); + throw "Error getting name property of file";Please define an exception.
Quoted 8 lines
Quoted 3 lines
Why?
Because none of the standard exception types seemed to fit and string exceptions are not a good idea.
So I can do both to get a specific error out instead of one that could mean any of the files were duplicated, for example.
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
libpolicyrep/src/policy_package.cpp line 89
Quoted 5 lines
libpolicyrep/src/policy_package.cpp +3
-0+ } + if (strstr(filename, ".mod")) { + // found module, do stuffErr - "do stuff"?
Quoted 7 lines
Quoted 3 lines
There is no
module_readfunction right now so thats just a placeholder.There is if the module is source :)
nak.
Quoted 78 lines
Quoted 78 lines
Quoted 12 lines
Quoted 12 lines
libpolicyrep/src/policy_package.cpp +12
-0+ } else if (strstr(filename, ".file_contexts")) { + if (!impl->file_contexts.empty()) { + // Found more than one file containing in .file_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple file_contexts files in policy package"; + } + impl->file_contexts = std::string(fbuf); + } else if (strstr(filename, ".seusers")) { + if (!impl->seusers.empty()) { + // Found more than one file containing in .seusers, + // don't know how to handle that so bail for nowQuoted 11 lines
libpolicyrep/src/policy_package.cpp line 105
Quoted 6 lines
libpolicyrep/src/policy_package.cpp +4
-0+ xar_close(x); + throw "Multiple seusers files in policy package"; + } + impl->seusers = std::string(fbuf);As we discussed privately - these copies are not great. Perhaps just use raw char*.
Ok, I don't know if this code is ever going to do anything with those and was trying to keep it in a format that would be more accessible if we needed to. I can change to char * though and see if anything develops that makes
std::stringmake more sense.Quoted 52 lines
Quoted 34 lines
libpolicyrep/src/policy_package.cpp +32
-0+ } else if (strstr(filename, ".user_extra")) { + if (!impl->user_extra.empty()) { + // Found more than one file containing in .user_extra, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple user_extra files in policy package"; + } + impl->user_extra = std::string(fbuf); + } else if (strstr(filename, ".netfilter_contexts")) { + if (!impl->netfilter_contexts.empty()) { + // Found more than one file containing in .netfilter_contexts, + // don't know how to handle that so bail for now + xar_close(x); + throw "Multiple netfilter_contexts files in policy package"; + } + impl->netfilter_contexts = std::string(fbuf); + } else { + // unrecognized file, just skip it + continue; + } + + } + + xar_close(x); + + return 0; + } + + PolicyPackage::PolicyPackage() { + init(); + } +libpolicyrep/src/policy_package.cpp line 147
Quoted 10 lines
libpolicyrep/src/policy_package.cpp +10
-0+ PolicyPackage::PolicyPackage(std::string filename) { + init(); + read_package(filename); + } + + PolicyPackage::~PolicyPackage() { delete impl; } + + const Module PolicyPackage::get_policy_module() const { + return impl->policy_module; + }This should definitely not be by value. It should also likely not be const - this is how the user will retrieve the module after reading to use, correct? Then it shouldn't be const (they might want to much with it), should be a reference, and shouldn't be const for the class (as allowing users to change the module changes the object).
Quoted 9 lines
Quoted 3 lines
Fair enough. I was basically thinking users would read a module and make their own copies (eg., copying the data into a merged copy or something) but that isn't really how policyrep works so I'll change all these to be non-const.
One other note - if the caller is expected to take ownership for the lifetime of the object (which seems possible), then it should return a pointer or a
shared_ptr.
ok?
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
Quoted 26 lines · Joshua Brindle
Quoted 25 lines · Karl MacMillan
Quoted 2 lines · Joshua Brindle
Karl MacMillan wrote:
[...]
Quoted 18 lines
Quoted 10 lines
libpolicyrep/src/policy_package.cpp line 82
Quoted 6 lines
libpolicyrep/src/policy_package.cpp +4
-0+ // Currently we use the name of the file to decide what kind of file it is + // I am fairly uncomfortable with this but it allows us to use xar to create + // policy packages instead of having our own write_package function that + // assigns attributes to files instead of using names.I don't like this either - what is the alternative?
Ok, xar has an XML ToC for the file. It also provides an API to set arbitrary attributes for each file, so I can do something like selinuxfiletype =
netfilter_contexts. This means that we would not be able to use the command line xar to create packages (I was trying to make this as simple as possible). If we move to using attributes we'd have to have a custom packager again (alasemodule_package).
Quoted 4 lines
Quoted 2 lines
It would be simpler though, correct? And what about an unpackager?
Simpler in what way?
Simpler than our current semodule_package and all of the library support behind it.
Quoted 8 lines
This first patch I was trying to keep it dead
simple by not even requiring a custom packager and instead just using xar CLI. If we want to use more sophisticated things in xar (like per-file attributes) we'd need something custom. Even if we just make a subdocument (which is archive-wide in scope) and add some selinux bits to it like policy name that can still be done with the CLI.xar -xf foo.xar will unpackage it
Ok - then I vote for a custom packaging tool. If nothing else it can retain some similarity to the current tool syntax.
[..]
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
There is no
module_readfunction right now so thats just a placeholder.
Quoted 5 lines
Quoted 3 lines
There is if the module is source :)
nak.
Why? Last time we discussed it you wanted support for storing additional information but the only examples that you had were original filename and line number. This is already supported (it has to be for the M4 processing). So what else? If nothing else, why have an additional format?
Karl
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
Quoted 11 lines · Karl MacMillan
Quoted 6 lines
This first patch I was trying to keep it dead
simple by not even requiring a custom packager and instead just using xar CLI. If we want to use more sophisticated things in xar (like per-file attributes) we'd need something custom. Even if we just make a subdocument (which is archive-wide in scope) and add some selinux bits to it like policy name that can still be done with the CLI.xar -xf foo.xar will unpackage it
Ok - then I vote for a custom packaging tool. If nothing else it can retain some similarity to the current tool syntax.
Ok, I'll go ahead and do that for the next version of the patches. We are basically taking a standard archive format and one-offing it, fortunately the format supports that kind of thing so we can continue using all their library infrastructure, which is a huge win over what we are doing now.
Quoted 14 lines
[..]
Quoted 11 lines
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
There is no
module_readfunction right now so thats just a placeholder.There is if the module is source :)
Quoted 9 lines
Quoted 2 lines
nak.
Why? Last time we discussed it you wanted support for storing additional information but the only examples that you had were original filename and line number. This is already supported (it has to be for the M4 processing). So what else? If nothing else, why have an additional format?
It doesn't seem reasonable to essentially reproduce the m4 preprocessing statements just to keep a single parser. just look at a preprocessed module and see how nasty it looks:
#line 156537 "tmp/all_interfaces.conf"
#line 1 "policy/modules/services/sendmail.te"
#line 2
#line 2
module sendmail 1.3.1;
#line 2
#line 2
require {
#line 2
role system_r;#line 2
That isn't exactly what I had in mind when I said we'd want to store extra info. We'd also have to add parser support for that mess to the source parser, non-ideal I think when we can path the data in something easily readable (by machine) without loads of junk that isn't necessary.
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
Quoted 1 line · Joshua Brindle
Karl MacMillan wrote:
[...]
Quoted 13 lines
It doesn't seem reasonable to essentially reproduce the m4 preprocessing statements just to keep a single parser. just look at a preprocessed module and see how nasty it looks:
#line 156537 "
tmp/all_interfaces.conf"#line 1 "policy/modules/services/sendmail.te"
#line 2
#line 2
Quoted 9 lines
module sendmail 1.3.1;#line 2
#line 2
require { #line 2 role system_r;#line 2
1) so what?
2) it doesn't have to be that ugly - the repeated line number thing is an artifact of M4.
Quoted 6 lines
That isn't exactly what I had in mind when I said we'd want to store extra info. We'd also have to add parser support for that mess to the source parser, non-ideal I think when we can path the data in something easily readable (by machine) without loads of junk that isn't necessary.
But the lexer already supports these - it's done. I just can't see inventing an entirely new file format just because you think this is "ugly". It works, it's there now, and we can make it prettier.
Karl
--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation
Quoted 28 lines · Karl MacMillan
Quoted 1 line · Joshua Brindle
Karl MacMillan wrote:
[...]
Quoted 13 lines
It doesn't seem reasonable to essentially reproduce the m4 preprocessing statements just to keep a single parser. just look at a preprocessed module and see how nasty it looks:
#line 156537 "
tmp/all_interfaces.conf"#line 1 "policy/modules/services/sendmail.te"
#line 2
#line 2
Quoted 9 lines
module sendmail 1.3.1;#line 2
#line 2
require { #line 2 role system_r;#line 2
Quoted 3 lines
1) so what?
2) it doesn't have to be that ugly - the repeated line number thing is an artifact of M4.
The line number and file name comments are optional in M4 and only turned on (via M4's -s option) since the current compiler can leverage them for the file name and line number in error messages.
Signature · 9 lines
Chris PeBenito Tresys Technology, LLC (410) 290-1411 x150 -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.
PATCH 3/3 policy package tests
In reply to [email protected]--
libpolicyrep/tests/libpolicyrep-test.cpp +22 -0
@@ -20,6 +20,7 @@ #include <policyrep/policy.hpp> #include <policyrep/parse.hpp> +#include <policyrep/policy_package.hpp> #include <sstream> #include <iostream>
@@ -57,6 +58,27 @@ parsed_mod->append_children(mod->children().begin(), mod->children().end()); + // policy package teddsts + std::cout << "============ Reading policy package ===========" << std::endl; + + try { + PolicyPackagePtr polpackage(new PolicyPackage("policy_package/test.xar")); + const std::string fc = polpackage->get_file_contexts(); + if (fc.empty()) + std::cout << "Error, file_contexts file not in package" << std::endl; + const std::string su = polpackage->get_seusers(); + if (su.empty()) + std::cout << "Error, seusers file not in package" << std::endl; + const std::string ue = polpackage->get_user_extra(); + if (ue.empty()) + std::cout << "Error, user_extra file not in package" << std::endl; + const std::string nc = polpackage->get_netfilter_contexts(); + if (nc.empty()) + std::cout << "Error, netfilter_contexts file not in package" << std::endl; + } catch (const char *e) { + std::cout << "Exception thrown: " << e << std::endl; + } + } int main(int argc, char **argv)
libpolicyrep/tests/Makefile +1 -1
@@ -12,7 +12,7 @@ all: $(EXE) $(EXE): $(objs) $(LIBPOLICYREP) - g++ $(CFLAGS) $(objs) $(LIBPOLICYREP) -lboost_serialization -o $@ + g++ $(CFLAGS) $(objs) $(LIBPOLICYREP) -lboost_serialization -lxar -o $@ %.o: %.cpp g++ $(CFLAGS) -fPIC -c -o $@ $<
Signature · 4 lines
-- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.