| Message ID | 20190206194325.24875-1-plautrba@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] python/semanage module: Fix handling of -a/-e/-d/-r options | expand |
On Wed, Feb 6, 2019 at 8:43 PM Petr Lautrbach <plautrba@redhat.com> wrote: > > Previous code traceback-ed when one of the mentioned option was used without > any argument as this state was not handled by the argument parser. > > action='store' stores arguments as a list while the original > action='store_const' used str therefore the particular interfaces in > moduleRecords are changed to be compatible with both. > > Fixes: > ^_^ semanage module -a > Traceback (most recent call last): > File "/usr/sbin/semanage", line 963, in <module> > do_parser() > File "/usr/sbin/semanage", line 942, in do_parser > args.func(args) > File "/usr/sbin/semanage", line 608, in handleModule > OBJECT.add(args.module_name, args.priority) > File "/usr/lib/python3.7/site-packages/seobject.py", line 402, in add > if not os.path.exists(file): > File "/usr/lib64/python3.7/genericpath.py", line 19, in exists > os.stat(path) > TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType > > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> Nice bug :) Nevertheless "if type(module) == str" troubles me because I except a function to only accept one kind of arguments (either a list of strings or a string, but not both). Moreover this is Python3-only code and semanage's shebang does not specify a Python version (the Python2-equivalent code would have been "if isinstance(module, basestring)"). I would prefer if the new code looked like this (that I have not tested): def set_enabled(self, modules, enable): for item_modules in modules: for m in item_modules.split(): # ... Moreover the "file = file[0]" in moduleRecords.add() looks strange without a context, which is in handleModule(). I would prefer if this operation occurred in semanage, where it is clear that args.action_add always has a single item (because « action='store', nargs=1 »): if args.action_add: OBJECT.add(args.action_add[0], args.priority) Nicolas PS: As setools is now Python3-only and seobject.py requires it, it seems to be a good time to update the shebang to "#!/usr/bin/python3 -Es". > --- > python/semanage/semanage | 25 ++++++++++++------------- > python/semanage/seobject.py | 10 ++++++++-- > 2 files changed, 20 insertions(+), 15 deletions(-) > > diff --git a/python/semanage/semanage b/python/semanage/semanage > index 6afeac14..9b737fa8 100644 > --- a/python/semanage/semanage > +++ b/python/semanage/semanage > @@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers): > > def handleModule(args): > OBJECT = seobject.moduleRecords(args) > - if args.action == "add": > - OBJECT.add(args.module_name, args.priority) > - if args.action == "enable": > - OBJECT.set_enabled(args.module_name, True) > - if args.action == "disable": > - OBJECT.set_enabled(args.module_name, False) > - if args.action == "remove": > - OBJECT.delete(args.module_name, args.priority) > + if args.action_add: > + OBJECT.add(args.action_add, args.priority) > + if args.action_enable: > + OBJECT.set_enabled(args.action_enable, True) > + if args.action_disable: > + OBJECT.set_enabled(args.action_disable, False) > + if args.action_remove: > + OBJECT.delete(args.action_remove, args.priority) > if args.action == "deleteall": > OBJECT.deleteall() > if args.action == "list": > @@ -635,14 +635,13 @@ def setupModuleParser(subparsers): > parser_add_priority(moduleParser, "module") > > mgroup = moduleParser.add_mutually_exclusive_group(required=True) > - parser_add_add(mgroup, "module") > parser_add_list(mgroup, "module") > parser_add_extract(mgroup, "module") > parser_add_deleteall(mgroup, "module") > - mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module")) > - mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module")) > - mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module")) > - moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on')) > + mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module")) > + mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module")) > + mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module")) > + mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module")) > moduleParser.set_defaults(func=handleModule) > > > diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py > index 556d3ba5..cd2d3457 100644 > --- a/python/semanage/seobject.py > +++ b/python/semanage/seobject.py > @@ -401,6 +401,8 @@ class moduleRecords(semanageRecords): > print("%-25s %-9s %-5s %s" % (t[0], t[2], t[3], disabled)) > > def add(self, file, priority): > + if type(file) == list: > + file = file[0] > if not os.path.exists(file): > raise ValueError(_("Module does not exist: %s ") % file) > > @@ -413,7 +415,9 @@ class moduleRecords(semanageRecords): > self.commit() > > def set_enabled(self, module, enable): > - for m in module.split(): > + if type(module) == str: > + module = module.split() > + for m in module: > rc, key = semanage_module_key_create(self.sh) > if rc < 0: > raise ValueError(_("Could not create module key")) > @@ -435,7 +439,9 @@ class moduleRecords(semanageRecords): > if rc < 0: > raise ValueError(_("Invalid priority %d (needs to be between 1 and 999)") % priority) > > - for m in module.split(): > + if type(module) == str: > + module = module.split() > + for m in module: > rc = semanage_module_remove(self.sh, m) > if rc < 0 and rc != -2: > raise ValueError(_("Could not remove module %s (remove failed)") % m) > -- > 2.20.1 >
Nicolas Iooss <nicolas.iooss@m4x.org> writes: > On Wed, Feb 6, 2019 at 8:43 PM Petr Lautrbach <plautrba@redhat.com> wrote: >> >> Previous code traceback-ed when one of the mentioned option was used without >> any argument as this state was not handled by the argument parser. >> >> action='store' stores arguments as a list while the original >> action='store_const' used str therefore the particular interfaces in >> moduleRecords are changed to be compatible with both. >> >> Fixes: >> ^_^ semanage module -a >> Traceback (most recent call last): >> File "/usr/sbin/semanage", line 963, in <module> >> do_parser() >> File "/usr/sbin/semanage", line 942, in do_parser >> args.func(args) >> File "/usr/sbin/semanage", line 608, in handleModule >> OBJECT.add(args.module_name, args.priority) >> File "/usr/lib/python3.7/site-packages/seobject.py", line 402, in add >> if not os.path.exists(file): >> File "/usr/lib64/python3.7/genericpath.py", line 19, in exists >> os.stat(path) >> TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType >> >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > > Nice bug :) Nevertheless "if type(module) == str" troubles me because > I except a function to only accept one kind of arguments (either a > list of strings or a string, but not both). The idea was to have backward compatible code. Originally, semanage used to send a string, while the new code sends a list. I didn't want to break 3rd party and I wanted to avoid converting of the list from action_enable to string and the converting it back to list in moduleRecords.set_enabled() > Moreover this is > Python3-only code and semanage's shebang does not specify a Python > version (the Python2-equivalent code would have been "if > isinstance(module, basestring)"). Works for me: > python2 Python 2.7.15 (default, Feb 2 2019, 16:04:51) [GCC 9.0.1 20190129 (Red Hat 9.0.1-0.2)] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> type("name") <type 'str'> >>> type(["name"]) <type 'list'> >>> type("name") == str True >>> type(["name"]) == str False https://docs.python.org/2/library/functions.html#type > > I would prefer if the new code looked like this (that I have not tested): > > def set_enabled(self, modules, enable): > for item_modules in modules: > for m in item_modules.split(): > # ... Or just convert action_enable to string before it's sent to moduleRecords.set_enabled(): --- a/python/semanage/semanage +++ b/python/semanage/semanage @@ -612,9 +612,9 @@ def handleModule(args): if args.action_add: OBJECT.add(args.action_add, args.priority) if args.action_enable: - OBJECT.set_enabled(args.action_enable, True) + OBJECT.set_enabled(args.action_enable.join(" "), True) if args.action_disable: - OBJECT.set_enabled(args.action_disable, False) + OBJECT.set_enabled(args.action_disable.join(" "), False) if args.action_remove: OBJECT.delete(args.action_remove, args.priority) > Moreover the "file = file[0]" in moduleRecords.add() looks strange > without a context, which is in handleModule(). I would prefer if this > operation occurred in semanage, where it is clear that args.action_add > always has a single item (because « action='store', nargs=1 »): > > if args.action_add: > OBJECT.add(args.action_add[0], args.priority) Good idea. > > Nicolas > > PS: As setools is now Python3-only and seobject.py requires it, it > seems to be a good time to update the shebang to "#!/usr/bin/python3 > -Es". seobject.py is just a module which is not supposed to be entry point. The shebang does not make sense here and should be removed. But I'd change at least semanage and chcat as they both directly imports seobject. Or rather change the whole project to python3 by default. > >> --- >> python/semanage/semanage | 25 ++++++++++++------------- >> python/semanage/seobject.py | 10 ++++++++-- >> 2 files changed, 20 insertions(+), 15 deletions(-) >> >> diff --git a/python/semanage/semanage b/python/semanage/semanage >> index 6afeac14..9b737fa8 100644 >> --- a/python/semanage/semanage >> +++ b/python/semanage/semanage >> @@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers): >> >> def handleModule(args): >> OBJECT = seobject.moduleRecords(args) >> - if args.action == "add": >> - OBJECT.add(args.module_name, args.priority) >> - if args.action == "enable": >> - OBJECT.set_enabled(args.module_name, True) >> - if args.action == "disable": >> - OBJECT.set_enabled(args.module_name, False) >> - if args.action == "remove": >> - OBJECT.delete(args.module_name, args.priority) >> + if args.action_add: >> + OBJECT.add(args.action_add, args.priority) >> + if args.action_enable: >> + OBJECT.set_enabled(args.action_enable, True) >> + if args.action_disable: >> + OBJECT.set_enabled(args.action_disable, False) >> + if args.action_remove: >> + OBJECT.delete(args.action_remove, args.priority) >> if args.action == "deleteall": >> OBJECT.deleteall() >> if args.action == "list": >> @@ -635,14 +635,13 @@ def setupModuleParser(subparsers): >> parser_add_priority(moduleParser, "module") >> >> mgroup = moduleParser.add_mutually_exclusive_group(required=True) >> - parser_add_add(mgroup, "module") >> parser_add_list(mgroup, "module") >> parser_add_extract(mgroup, "module") >> parser_add_deleteall(mgroup, "module") >> - mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module")) >> - mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module")) >> - mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module")) >> - moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on')) >> + mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module")) >> + mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module")) >> + mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module")) >> + mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module")) >> moduleParser.set_defaults(func=handleModule) >> >> >> diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py >> index 556d3ba5..cd2d3457 100644 >> --- a/python/semanage/seobject.py >> +++ b/python/semanage/seobject.py >> @@ -401,6 +401,8 @@ class moduleRecords(semanageRecords): >> print("%-25s %-9s %-5s %s" % (t[0], t[2], t[3], disabled)) >> >> def add(self, file, priority): >> + if type(file) == list: >> + file = file[0] >> if not os.path.exists(file): >> raise ValueError(_("Module does not exist: %s ") % file) >> >> @@ -413,7 +415,9 @@ class moduleRecords(semanageRecords): >> self.commit() >> >> def set_enabled(self, module, enable): >> - for m in module.split(): >> + if type(module) == str: >> + module = module.split() >> + for m in module: >> rc, key = semanage_module_key_create(self.sh) >> if rc < 0: >> raise ValueError(_("Could not create module key")) >> @@ -435,7 +439,9 @@ class moduleRecords(semanageRecords): >> if rc < 0: >> raise ValueError(_("Invalid priority %d (needs to be between 1 and 999)") % priority) >> >> - for m in module.split(): >> + if type(module) == str: >> + module = module.split() >> + for m in module: >> rc = semanage_module_remove(self.sh, m) >> if rc < 0 and rc != -2: >> raise ValueError(_("Could not remove module %s (remove failed)") % m) >> -- >> 2.20.1 >>
On Fri, Feb 15, 2019 at 3:28 PM Petr Lautrbach <plautrba@redhat.com> wrote: > > Nicolas Iooss <nicolas.iooss@m4x.org> writes: > > > On Wed, Feb 6, 2019 at 8:43 PM Petr Lautrbach <plautrba@redhat.com> wrote: > >> > >> Previous code traceback-ed when one of the mentioned option was used without > >> any argument as this state was not handled by the argument parser. > >> > >> action='store' stores arguments as a list while the original > >> action='store_const' used str therefore the particular interfaces in > >> moduleRecords are changed to be compatible with both. > >> > >> Fixes: > >> ^_^ semanage module -a > >> Traceback (most recent call last): > >> File "/usr/sbin/semanage", line 963, in <module> > >> do_parser() > >> File "/usr/sbin/semanage", line 942, in do_parser > >> args.func(args) > >> File "/usr/sbin/semanage", line 608, in handleModule > >> OBJECT.add(args.module_name, args.priority) > >> File "/usr/lib/python3.7/site-packages/seobject.py", line 402, in add > >> if not os.path.exists(file): > >> File "/usr/lib64/python3.7/genericpath.py", line 19, in exists > >> os.stat(path) > >> TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType > >> > >> Signed-off-by: Petr Lautrbach <plautrba@redhat.com> > > > > Nice bug :) Nevertheless "if type(module) == str" troubles me because > > I except a function to only accept one kind of arguments (either a > > list of strings or a string, but not both). > > The idea was to have backward compatible code. Originally, semanage used > to send a string, while the new code sends a list. I didn't want to > break 3rd party and I wanted to avoid converting of the list from > action_enable to string and the converting it back to list in > moduleRecords.set_enabled() > > > > Moreover this is > > Python3-only code and semanage's shebang does not specify a Python > > version (the Python2-equivalent code would have been "if > > isinstance(module, basestring)"). > > Works for me: > > > python2 > Python 2.7.15 (default, Feb 2 2019, 16:04:51) > [GCC 9.0.1 20190129 (Red Hat 9.0.1-0.2)] on linux2 > Type "help", "copyright", "credits" or "license" for more information. > >>> type("name") > <type 'str'> > >>> type(["name"]) > <type 'list'> > >>> type("name") == str > True > >>> type(["name"]) == str > False > > > https://docs.python.org/2/library/functions.html#type I had unicode strings in mind: python2 >>> type(u'abc') <type 'unicode'> >>> type(u'abc') == str False >>> isinstance(u'abc', basestring) True Nevertheless, it seems that argparse does not introduce unicode strings when UTF-8 characters are provided as command-line parameters, so your code worked. > > > > I would prefer if the new code looked like this (that I have not tested): > > > > def set_enabled(self, modules, enable): > > for item_modules in modules: > > for m in item_modules.split(): > > # ... > > Or just convert action_enable to string before it's sent to > moduleRecords.set_enabled(): > > --- a/python/semanage/semanage > +++ b/python/semanage/semanage > @@ -612,9 +612,9 @@ def handleModule(args): > if args.action_add: > OBJECT.add(args.action_add, args.priority) > if args.action_enable: > - OBJECT.set_enabled(args.action_enable, True) > + OBJECT.set_enabled(args.action_enable.join(" "), True) > if args.action_disable: > - OBJECT.set_enabled(args.action_disable, False) > + OBJECT.set_enabled(args.action_disable.join(" "), False) > if args.action_remove: > OBJECT.delete(args.action_remove, args.priority) Indeed. I like it better, thanks! > > Moreover the "file = file[0]" in moduleRecords.add() looks strange > > without a context, which is in handleModule(). I would prefer if this > > operation occurred in semanage, where it is clear that args.action_add > > always has a single item (because « action='store', nargs=1 »): > > > > if args.action_add: > > OBJECT.add(args.action_add[0], args.priority) > > Good idea. > > > > > Nicolas > > > > PS: As setools is now Python3-only and seobject.py requires it, it > > seems to be a good time to update the shebang to "#!/usr/bin/python3 > > -Es". > > seobject.py is just a module which is not supposed to be entry point. > The shebang does not make sense here and should be removed. > > But I'd change at least semanage and chcat as they both directly imports > seobject. > > Or rather change the whole project to python3 by default. I agree with changing all the shebangs in the project to python3 (as python/sepolicy/sepolicy/__init__.py also imports setools, there are more tools that are broken with a Python3-only setools and a Python2 "python" binary). Nicolas
diff --git a/python/semanage/semanage b/python/semanage/semanage index 6afeac14..9b737fa8 100644 --- a/python/semanage/semanage +++ b/python/semanage/semanage @@ -609,14 +609,14 @@ def setupInterfaceParser(subparsers): def handleModule(args): OBJECT = seobject.moduleRecords(args) - if args.action == "add": - OBJECT.add(args.module_name, args.priority) - if args.action == "enable": - OBJECT.set_enabled(args.module_name, True) - if args.action == "disable": - OBJECT.set_enabled(args.module_name, False) - if args.action == "remove": - OBJECT.delete(args.module_name, args.priority) + if args.action_add: + OBJECT.add(args.action_add, args.priority) + if args.action_enable: + OBJECT.set_enabled(args.action_enable, True) + if args.action_disable: + OBJECT.set_enabled(args.action_disable, False) + if args.action_remove: + OBJECT.delete(args.action_remove, args.priority) if args.action == "deleteall": OBJECT.deleteall() if args.action == "list": @@ -635,14 +635,13 @@ def setupModuleParser(subparsers): parser_add_priority(moduleParser, "module") mgroup = moduleParser.add_mutually_exclusive_group(required=True) - parser_add_add(mgroup, "module") parser_add_list(mgroup, "module") parser_add_extract(mgroup, "module") parser_add_deleteall(mgroup, "module") - mgroup.add_argument('-r', '--remove', dest='action', action='store_const', const='remove', help=_("Remove a module")) - mgroup.add_argument('-d', '--disable', dest='action', action='store_const', const='disable', help=_("Disable a module")) - mgroup.add_argument('-e', '--enable', dest='action', action='store_const', const='enable', help=_("Enable a module")) - moduleParser.add_argument('module_name', nargs='?', default=None, help=_('Name of the module to act on')) + mgroup.add_argument('-a', '--add', dest='action_add', action='store', nargs=1, metavar='module_name', help=_("Add a module")) + mgroup.add_argument('-r', '--remove', dest='action_remove', action='store', nargs='+', metavar='module_name', help=_("Remove a module")) + mgroup.add_argument('-d', '--disable', dest='action_disable', action='store', nargs='+', metavar='module_name', help=_("Disable a module")) + mgroup.add_argument('-e', '--enable', dest='action_enable', action='store', nargs='+', metavar='module_name', help=_("Enable a module")) moduleParser.set_defaults(func=handleModule) diff --git a/python/semanage/seobject.py b/python/semanage/seobject.py index 556d3ba5..cd2d3457 100644 --- a/python/semanage/seobject.py +++ b/python/semanage/seobject.py @@ -401,6 +401,8 @@ class moduleRecords(semanageRecords): print("%-25s %-9s %-5s %s" % (t[0], t[2], t[3], disabled)) def add(self, file, priority): + if type(file) == list: + file = file[0] if not os.path.exists(file): raise ValueError(_("Module does not exist: %s ") % file) @@ -413,7 +415,9 @@ class moduleRecords(semanageRecords): self.commit() def set_enabled(self, module, enable): - for m in module.split(): + if type(module) == str: + module = module.split() + for m in module: rc, key = semanage_module_key_create(self.sh) if rc < 0: raise ValueError(_("Could not create module key")) @@ -435,7 +439,9 @@ class moduleRecords(semanageRecords): if rc < 0: raise ValueError(_("Invalid priority %d (needs to be between 1 and 999)") % priority) - for m in module.split(): + if type(module) == str: + module = module.split() + for m in module: rc = semanage_module_remove(self.sh, m) if rc < 0 and rc != -2: raise ValueError(_("Could not remove module %s (remove failed)") % m)
Previous code traceback-ed when one of the mentioned option was used without any argument as this state was not handled by the argument parser. action='store' stores arguments as a list while the original action='store_const' used str therefore the particular interfaces in moduleRecords are changed to be compatible with both. Fixes: ^_^ semanage module -a Traceback (most recent call last): File "/usr/sbin/semanage", line 963, in <module> do_parser() File "/usr/sbin/semanage", line 942, in do_parser args.func(args) File "/usr/sbin/semanage", line 608, in handleModule OBJECT.add(args.module_name, args.priority) File "/usr/lib/python3.7/site-packages/seobject.py", line 402, in add if not os.path.exists(file): File "/usr/lib64/python3.7/genericpath.py", line 19, in exists os.stat(path) TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType Signed-off-by: Petr Lautrbach <plautrba@redhat.com> --- python/semanage/semanage | 25 ++++++++++++------------- python/semanage/seobject.py | 10 ++++++++-- 2 files changed, 20 insertions(+), 15 deletions(-)