[libvirt PATCH] run: add ability to set selinux context
When running libvirt from the build directory with the 'run' script, it will run as unconfined_t. This can result in unexpected behavior when selinux is enforcing due to the fact that the selinux policies are written assuming that libvirt is running with the system_u:system_r:virtd_t context. This patch adds a new --selinux option to the run script. When this option is specified, it will launch the specified binary using the 'runcon' utility to set its selinux context to the one mentioned above. Since this requires root privileges, setting the selinux context is not the default behavior and must be enabled with the command line switch. Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- run.in | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/run.in b/run.in index c6d3411082..4aa458b791 100644 --- a/run.in +++ b/run.in @@ -40,6 +40,7 @@ # # ---------------------------------------------------------------------- +import argparse import os import os.path import random @@ -59,15 +60,20 @@ def prepend(env, varname, extradir): here = "@abs_builddir@" -if len(sys.argv) < 2: - print("syntax: %s BINARY [ARGS...]" % sys.argv[0], file=sys.stderr) +parser = argparse.ArgumentParser(add_help=False) +parser.add_argument('--selinux', + action='store_true', + help='Run in the appropriate selinux context') + +opts, args = parser.parse_known_args() + +if len(args) < 1: + print("syntax: %s [--selinux] BINARY [ARGS...]" % sys.argv[0], file=sys.stderr) sys.exit(1) -prog = sys.argv[1] -args = sys.argv[1:] +prog = args[0] env = os.environ - prepend(env, "LD_LIBRARY_PATH", os.path.join(here, "src")) prepend(env, "PKG_CONFIG_PATH", os.path.join(here, "src")) prepend(env, "PATH", os.path.join(here, "tools")) @@ -130,10 +136,20 @@ def change_unit(name, action): return ret == 0 +def chcon(path, type): + ret = subprocess.call(["chcon", "-t", type, path]) + return ret == 0 + + +def restorecon(path): + ret = subprocess.call(["restorecon", path]) + return ret == 0 + + try_stop_units = [] if is_systemd_host(): maybe_stopped_units = [] - for arg in sys.argv: + for arg in args: name = os.path.basename(arg) if is_modular_daemon(name): # Only need to stop libvirtd or this specific modular unit @@ -149,6 +165,31 @@ if is_systemd_host(): if is_unit_active(unit): try_stop_units.append(unit) +if opts.selinux: + # if using a wrapper command like 'gdb', setting the selinux context + # won't work because the wrapper command will not be a valid entrypoint + # for the virtd_t context + if os.path.basename(prog) not in ["libvirtd", *modular_daemons]: + sys.exit("'{}' is not recognized as a valid libvirt daemon. Selinux " + "process context can only be set when using executing a " + "libvirt daemon directly without wrapper commands".format(prog)) + + progpath = os.path.abspath(prog) + if not progpath.startswith(os.path.abspath(here)): + sys.exit("Refusing to change selinux context of file outside build " + "directory: {}".format(prog)) + + # selinux won't allow us to transition to the virtd_t context from e.g. the + # user_home_t context (the likely label of the local executable file) + print("Setting file context of {} to virtd_exec_t...".format(progpath)) + if not chcon(progpath, "virtd_exec_t"): + sys.exit("Failed to change selinux context of binary") + + args = ['runcon', + '-u', 'system_u', + '-r', 'system_r', + '-t', 'virtd_t', *args] + if len(try_stop_units) == 0: # Run the program directly, replacing ourselves os.execvpe(prog, args, env) @@ -178,6 +219,9 @@ else: except Exception as e: print("%s" % e, file=sys.stderr) finally: + if opts.selinux: + print("Restoring selinux context...") + restorecon(prog) print("Re-starting original systemd units...") stopped_units.reverse() for unit in stopped_units: -- 2.39.2
On Mon, Apr 24, 2023 at 03:50:48PM -0500, Jonathon Jongsma wrote:
When running libvirt from the build directory with the 'run' script, it will run as unconfined_t. This can result in unexpected behavior when selinux is enforcing due to the fact that the selinux policies are written assuming that libvirt is running with the system_u:system_r:virtd_t context. This patch adds a new --selinux option to the run script. When this option is specified, it will launch the specified binary using the 'runcon' utility to set its selinux context to the one mentioned above. Since this requires root privileges, setting the selinux context is not the default behavior and must be enabled with the command line switch.
A fiddled with writing custom selinux transition rules to achieve the same thing a couple years back, but never finished it. No wonder, this is a much cleaner approach. I will only comment on the Python side of things, leaving the overall approach and idea commenting to someone else.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- run.in | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/run.in b/run.in index c6d3411082..4aa458b791 100644 --- a/run.in +++ b/run.in @@ -40,6 +40,7 @@ # # ----------------------------------------------------------------------
+import argparse import os import os.path import random @@ -59,15 +60,20 @@ def prepend(env, varname, extradir):
here = "@abs_builddir@"
-if len(sys.argv) < 2: - print("syntax: %s BINARY [ARGS...]" % sys.argv[0], file=sys.stderr)
Since you decided to use argparse (yes please), we can drop ^this if we properly set the arguments with argparse, it'll even generate the help for us. This way it looks only like a partial solution. Argparse has great documentation so you can just take one of the examples they list.
+parser = argparse.ArgumentParser(add_help=False)
Why don't we want the automatic help?
+parser.add_argument('--selinux', + action='store_true', + help='Run in the appropriate selinux context') + +opts, args = parser.parse_known_args()
If you want to use ^this, then you need to be aware of prefix matching on the options recognized by Argparse. IOW if one is to pass <args> to the <binary> then none of the <args> can be a prefix of any of the long options argeparse knows about (in this case --selinux), otherwise it'll consume it. Altough unlikely, we should stay on the safe side and use: argparse.ArgumentParser(..., allow_abbrev=False) [1] [2] https://docs.python.org/3.11/library/argparse.html?highlight=argparse#allow-...
+ +if len(args) < 1: + print("syntax: %s [--selinux] BINARY [ARGS...]" % sys.argv[0], file=sys.stderr) sys.exit(1)
Same here, with argparse ^this is not needed if the args/options are defined correctly.
-prog = sys.argv[1] -args = sys.argv[1:] +prog = args[0]
argparse's parser obj has a 'prog' attribute [2]. [2] https://docs.python.org/3/library/argparse.html#prog The rest looks good from Python POV, but like I said, although I'm up for this idea, I'll let someone else comment on that. Regards, Erik
On 4/25/23 8:11 AM, Erik Skultety wrote:
On Mon, Apr 24, 2023 at 03:50:48PM -0500, Jonathon Jongsma wrote:
When running libvirt from the build directory with the 'run' script, it will run as unconfined_t. This can result in unexpected behavior when selinux is enforcing due to the fact that the selinux policies are written assuming that libvirt is running with the system_u:system_r:virtd_t context. This patch adds a new --selinux option to the run script. When this option is specified, it will launch the specified binary using the 'runcon' utility to set its selinux context to the one mentioned above. Since this requires root privileges, setting the selinux context is not the default behavior and must be enabled with the command line switch.
A fiddled with writing custom selinux transition rules to achieve the same thing a couple years back, but never finished it. No wonder, this is a much cleaner approach. I will only comment on the Python side of things, leaving the overall approach and idea commenting to someone else.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- run.in | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/run.in b/run.in index c6d3411082..4aa458b791 100644 --- a/run.in +++ b/run.in @@ -40,6 +40,7 @@ # # ----------------------------------------------------------------------
+import argparse import os import os.path import random @@ -59,15 +60,20 @@ def prepend(env, varname, extradir):
here = "@abs_builddir@"
-if len(sys.argv) < 2: - print("syntax: %s BINARY [ARGS...]" % sys.argv[0], file=sys.stderr)
Since you decided to use argparse (yes please), we can drop ^this if we properly set the arguments with argparse, it'll even generate the help for us. This way it looks only like a partial solution. Argparse has great documentation so you can just take one of the examples they list.
Yeah, I probably should have commented on why I used this 'partial' approach. I tried a few different ways, including adding a positional argument to argparse that would capture the target executable and its arguments like so: argsparse.add_argument("args", nargs="+") and then parsing with parser.parse_args() rather than parse_known_args(). But that prevented me from passing arguments to the target binary without inserting a '--' in to indicate that the run script should stop parsing: Fails: # ./_build/run --selinux ./_build/src/libvirtd --verbose usage: run [--selinux] args [args ...] run: error: unrecognized arguments: --verbose Works: # ./_build/run --selinux -- ./_build/src/libvirtd --verbose 2023-04-25 14:26:32.175+0000: 1530463: info : libvirt version: 9.3.0 ... That seemed annoying to me. Maybe we could keep using parse_known_args() with the 'args' argument defined above, but I have a vague recollection that this caused some other undesirable behavior so I switched back to the version I submitted. I'll try to refresh my memory.
+parser = argparse.ArgumentParser(add_help=False)
Why don't we want the automatic help?
Because then the run script would intercept the --help option and prevent us from passing it to e.g. libvirt or virsh. Maybe that's not something that we really care about, but I made the choice to pass as much through to the executable as possible.
+parser.add_argument('--selinux', + action='store_true', + help='Run in the appropriate selinux context') + +opts, args = parser.parse_known_args()
If you want to use ^this, then you need to be aware of prefix matching on the options recognized by Argparse. IOW if one is to pass <args> to the <binary> then none of the <args> can be a prefix of any of the long options argeparse knows about (in this case --selinux), otherwise it'll consume it. Altough unlikely, we should stay on the safe side and use: argparse.ArgumentParser(..., allow_abbrev=False) [1]
[2] https://docs.python.org/3.11/library/argparse.html?highlight=argparse#allow-...
ok, I wasn't aware of that option.
+ +if len(args) < 1: + print("syntax: %s [--selinux] BINARY [ARGS...]" % sys.argv[0], file=sys.stderr) sys.exit(1)
Same here, with argparse ^this is not needed if the args/options are defined correctly.
-prog = sys.argv[1] -args = sys.argv[1:] +prog = args[0]
argparse's parser obj has a 'prog' attribute [2].
I think that's the wrong 'prog' though. That would give me the './run' script, whereas I want the 'libvirtd' program (or whatever) that the user wants to execute.
The rest looks good from Python POV, but like I said, although I'm up for this idea, I'll let someone else comment on that.
Regards, Erik
On 4/25/23 9:43 AM, Jonathon Jongsma wrote:
On 4/25/23 8:11 AM, Erik Skultety wrote:
On Mon, Apr 24, 2023 at 03:50:48PM -0500, Jonathon Jongsma wrote:
When running libvirt from the build directory with the 'run' script, it will run as unconfined_t. This can result in unexpected behavior when selinux is enforcing due to the fact that the selinux policies are written assuming that libvirt is running with the system_u:system_r:virtd_t context. This patch adds a new --selinux option to the run script. When this option is specified, it will launch the specified binary using the 'runcon' utility to set its selinux context to the one mentioned above. Since this requires root privileges, setting the selinux context is not the default behavior and must be enabled with the command line switch.
A fiddled with writing custom selinux transition rules to achieve the same thing a couple years back, but never finished it. No wonder, this is a much cleaner approach. I will only comment on the Python side of things, leaving the overall approach and idea commenting to someone else.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- run.in | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/run.in b/run.in index c6d3411082..4aa458b791 100644 --- a/run.in +++ b/run.in @@ -40,6 +40,7 @@ # # ---------------------------------------------------------------------- +import argparse import os import os.path import random @@ -59,15 +60,20 @@ def prepend(env, varname, extradir): here = "@abs_builddir@" -if len(sys.argv) < 2: - print("syntax: %s BINARY [ARGS...]" % sys.argv[0], file=sys.stderr)
Since you decided to use argparse (yes please), we can drop ^this if we properly set the arguments with argparse, it'll even generate the help for us. This way it looks only like a partial solution. Argparse has great documentation so you can just take one of the examples they list.
Yeah, I probably should have commented on why I used this 'partial' approach. I tried a few different ways, including adding a positional argument to argparse that would capture the target executable and its arguments like so:
argsparse.add_argument("args", nargs="+")
and then parsing with parser.parse_args() rather than parse_known_args(). But that prevented me from passing arguments to the target binary without inserting a '--' in to indicate that the run script should stop parsing:
Fails: # ./_build/run --selinux ./_build/src/libvirtd --verbose usage: run [--selinux] args [args ...] run: error: unrecognized arguments: --verbose
Works: # ./_build/run --selinux -- ./_build/src/libvirtd --verbose 2023-04-25 14:26:32.175+0000: 1530463: info : libvirt version: 9.3.0 ...
That seemed annoying to me.
Maybe we could keep using parse_known_args() with the 'args' argument defined above, but I have a vague recollection that this caused some other undesirable behavior so I switched back to the version I submitted. I'll try to refresh my memory.
So here's one case that becomes more annoying when using the above setup (the new 'args' catchall argument combined with parse_known_args()): ./run gdb -- libvirtd --verbose gdb: unrecognized option '--verbose' the ./run script eats the '--' that should go to gdb, so gdb tries to interpret the --verbose option rather than passing it on to libvirtd.
+parser = argparse.ArgumentParser(add_help=False)
Why don't we want the automatic help?
Because then the run script would intercept the --help option and prevent us from passing it to e.g. libvirt or virsh. Maybe that's not something that we really care about, but I made the choice to pass as much through to the executable as possible.
+parser.add_argument('--selinux', + action='store_true', + help='Run in the appropriate selinux context') + +opts, args = parser.parse_known_args()
If you want to use ^this, then you need to be aware of prefix matching on the options recognized by Argparse. IOW if one is to pass <args> to the <binary> then none of the <args> can be a prefix of any of the long options argeparse knows about (in this case --selinux), otherwise it'll consume it. Altough unlikely, we should stay on the safe side and use: argparse.ArgumentParser(..., allow_abbrev=False) [1]
[2] https://docs.python.org/3.11/library/argparse.html?highlight=argparse#allow-...
ok, I wasn't aware of that option.
+ +if len(args) < 1: + print("syntax: %s [--selinux] BINARY [ARGS...]" % sys.argv[0], file=sys.stderr) sys.exit(1)
Same here, with argparse ^this is not needed if the args/options are defined correctly.
-prog = sys.argv[1] -args = sys.argv[1:] +prog = args[0]
argparse's parser obj has a 'prog' attribute [2].
I think that's the wrong 'prog' though. That would give me the './run' script, whereas I want the 'libvirtd' program (or whatever) that the user wants to execute.
The rest looks good from Python POV, but like I said, although I'm up for this idea, I'll let someone else comment on that.
Regards, Erik
On Tue, Apr 25, 2023 at 10:40:45AM -0500, Jonathon Jongsma wrote:
On 4/25/23 9:43 AM, Jonathon Jongsma wrote:
On 4/25/23 8:11 AM, Erik Skultety wrote:
On Mon, Apr 24, 2023 at 03:50:48PM -0500, Jonathon Jongsma wrote:
When running libvirt from the build directory with the 'run' script, it will run as unconfined_t. This can result in unexpected behavior when selinux is enforcing due to the fact that the selinux policies are written assuming that libvirt is running with the system_u:system_r:virtd_t context. This patch adds a new --selinux option to the run script. When this option is specified, it will launch the specified binary using the 'runcon' utility to set its selinux context to the one mentioned above. Since this requires root privileges, setting the selinux context is not the default behavior and must be enabled with the command line switch.
A fiddled with writing custom selinux transition rules to achieve the same thing a couple years back, but never finished it. No wonder, this is a much cleaner approach. I will only comment on the Python side of things, leaving the overall approach and idea commenting to someone else.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- run.in | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/run.in b/run.in index c6d3411082..4aa458b791 100644 --- a/run.in +++ b/run.in @@ -40,6 +40,7 @@ # # ---------------------------------------------------------------------- +import argparse import os import os.path import random @@ -59,15 +60,20 @@ def prepend(env, varname, extradir): here = "@abs_builddir@" -if len(sys.argv) < 2: - print("syntax: %s BINARY [ARGS...]" % sys.argv[0], file=sys.stderr)
Since you decided to use argparse (yes please), we can drop ^this if we properly set the arguments with argparse, it'll even generate the help for us. This way it looks only like a partial solution. Argparse has great documentation so you can just take one of the examples they list.
Yeah, I probably should have commented on why I used this 'partial' approach. I tried a few different ways, including adding a positional argument to argparse that would capture the target executable and its arguments like so:
argsparse.add_argument("args", nargs="+")
and then parsing with parser.parse_args() rather than parse_known_args(). But that prevented me from passing arguments to the target binary without inserting a '--' in to indicate that the run script should stop parsing:
Fails: # ./_build/run --selinux ./_build/src/libvirtd --verbose usage: run [--selinux] args [args ...] run: error: unrecognized arguments: --verbose
Works: # ./_build/run --selinux -- ./_build/src/libvirtd --verbose 2023-04-25 14:26:32.175+0000: 1530463: info : libvirt version: 9.3.0 ...
That seemed annoying to me.
Maybe we could keep using parse_known_args() with the 'args' argument defined above, but I have a vague recollection that this caused some other undesirable behavior so I switched back to the version I submitted. I'll try to refresh my memory.
So here's one case that becomes more annoying when using the above setup (the new 'args' catchall argument combined with parse_known_args()):
./run gdb -- libvirtd --verbose gdb: unrecognized option '--verbose'
the ./run script eats the '--' that should go to gdb, so gdb tries to interpret the --verbose option rather than passing it on to libvirtd.
I see. Without trying it/playing with it myself, I don't have a better proposal at the moment, let me have a look at this tomorrow, I'll take your patch and see if anything I'm familiar with in argparse would work, if not or if you can figure out a better way in the meantime, I'll retract my comments to this patch, how about that? Regards, Erik
On 4/25/23 11:54 AM, Erik Skultety wrote:
On Tue, Apr 25, 2023 at 10:40:45AM -0500, Jonathon Jongsma wrote:
On 4/25/23 9:43 AM, Jonathon Jongsma wrote:
On 4/25/23 8:11 AM, Erik Skultety wrote:
On Mon, Apr 24, 2023 at 03:50:48PM -0500, Jonathon Jongsma wrote:
When running libvirt from the build directory with the 'run' script, it will run as unconfined_t. This can result in unexpected behavior when selinux is enforcing due to the fact that the selinux policies are written assuming that libvirt is running with the system_u:system_r:virtd_t context. This patch adds a new --selinux option to the run script. When this option is specified, it will launch the specified binary using the 'runcon' utility to set its selinux context to the one mentioned above. Since this requires root privileges, setting the selinux context is not the default behavior and must be enabled with the command line switch.
A fiddled with writing custom selinux transition rules to achieve the same thing a couple years back, but never finished it. No wonder, this is a much cleaner approach. I will only comment on the Python side of things, leaving the overall approach and idea commenting to someone else.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- run.in | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/run.in b/run.in index c6d3411082..4aa458b791 100644 --- a/run.in +++ b/run.in @@ -40,6 +40,7 @@ # # ---------------------------------------------------------------------- +import argparse import os import os.path import random @@ -59,15 +60,20 @@ def prepend(env, varname, extradir): here = "@abs_builddir@" -if len(sys.argv) < 2: - print("syntax: %s BINARY [ARGS...]" % sys.argv[0], file=sys.stderr)
Since you decided to use argparse (yes please), we can drop ^this if we properly set the arguments with argparse, it'll even generate the help for us. This way it looks only like a partial solution. Argparse has great documentation so you can just take one of the examples they list.
Yeah, I probably should have commented on why I used this 'partial' approach. I tried a few different ways, including adding a positional argument to argparse that would capture the target executable and its arguments like so:
argsparse.add_argument("args", nargs="+")
and then parsing with parser.parse_args() rather than parse_known_args(). But that prevented me from passing arguments to the target binary without inserting a '--' in to indicate that the run script should stop parsing:
Fails: # ./_build/run --selinux ./_build/src/libvirtd --verbose usage: run [--selinux] args [args ...] run: error: unrecognized arguments: --verbose
Works: # ./_build/run --selinux -- ./_build/src/libvirtd --verbose 2023-04-25 14:26:32.175+0000: 1530463: info : libvirt version: 9.3.0 ...
That seemed annoying to me.
Maybe we could keep using parse_known_args() with the 'args' argument defined above, but I have a vague recollection that this caused some other undesirable behavior so I switched back to the version I submitted. I'll try to refresh my memory.
So here's one case that becomes more annoying when using the above setup (the new 'args' catchall argument combined with parse_known_args()):
./run gdb -- libvirtd --verbose gdb: unrecognized option '--verbose'
the ./run script eats the '--' that should go to gdb, so gdb tries to interpret the --verbose option rather than passing it on to libvirtd.
I see. Without trying it/playing with it myself, I don't have a better proposal at the moment, let me have a look at this tomorrow, I'll take your patch and see if anything I'm familiar with in argparse would work, if not or if you can figure out a better way in the meantime, I'll retract my comments to this patch, how about that?
I did play around a little more and couldn't immediately find a better way to do it. If you find a better approach, I'm happy to consider it. I didn't initially consider using something like a a subparser (as you mentioned in a different email) because that would change the way that the script is invoked and I didn't want to disrupt others' development workflow. But if people are OK with that, then that obviously opens up more options. Jonathon
On Tue, Apr 25, 2023 at 09:43:38AM -0500, Jonathon Jongsma wrote:
On 4/25/23 8:11 AM, Erik Skultety wrote:
On Mon, Apr 24, 2023 at 03:50:48PM -0500, Jonathon Jongsma wrote:
When running libvirt from the build directory with the 'run' script, it will run as unconfined_t. This can result in unexpected behavior when selinux is enforcing due to the fact that the selinux policies are written assuming that libvirt is running with the system_u:system_r:virtd_t context. This patch adds a new --selinux option to the run script. When this option is specified, it will launch the specified binary using the 'runcon' utility to set its selinux context to the one mentioned above. Since this requires root privileges, setting the selinux context is not the default behavior and must be enabled with the command line switch.
A fiddled with writing custom selinux transition rules to achieve the same thing a couple years back, but never finished it. No wonder, this is a much cleaner approach. I will only comment on the Python side of things, leaving the overall approach and idea commenting to someone else.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- run.in | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 6 deletions(-)
diff --git a/run.in b/run.in index c6d3411082..4aa458b791 100644 --- a/run.in +++ b/run.in @@ -40,6 +40,7 @@ # # ---------------------------------------------------------------------- +import argparse import os import os.path import random @@ -59,15 +60,20 @@ def prepend(env, varname, extradir): here = "@abs_builddir@" -if len(sys.argv) < 2: - print("syntax: %s BINARY [ARGS...]" % sys.argv[0], file=sys.stderr)
Since you decided to use argparse (yes please), we can drop ^this if we properly set the arguments with argparse, it'll even generate the help for us. This way it looks only like a partial solution. Argparse has great documentation so you can just take one of the examples they list.
Yeah, I probably should have commented on why I used this 'partial' approach. I tried a few different ways, including adding a positional argument to argparse that would capture the target executable and its arguments like so:
argsparse.add_argument("args", nargs="+")
and then parsing with parser.parse_args() rather than parse_known_args(). But that prevented me from passing arguments to the target binary without inserting a '--' in to indicate that the run script should stop parsing:
Fails: # ./_build/run --selinux ./_build/src/libvirtd --verbose usage: run [--selinux] args [args ...] run: error: unrecognized arguments: --verbose
Works: # ./_build/run --selinux -- ./_build/src/libvirtd --verbose 2023-04-25 14:26:32.175+0000: 1530463: info : libvirt version: 9.3.0 ...
That seemed annoying to me.
Maybe we could keep using parse_known_args() with the 'args' argument defined above, but I have a vague recollection that this caused some other undesirable behavior so I switched back to the version I submitted. I'll try to refresh my memory.
+parser = argparse.ArgumentParser(add_help=False)
Why don't we want the automatic help?
Because then the run script would intercept the --help option and prevent us from passing it to e.g. libvirt or virsh. Maybe that's not something that we really care about, but I made the choice to pass as much through to the executable as possible.
Right, good point. Without trying it myself, could we utilize subparsers for this? IOW, have: ./run binary <binary> <args> where 'binary' is a keyword arg for the run script. Since --help is positional in argparse when you use subparsers, we could have the main parser with help, but the 'binary' subparser without it - that way we could theoretically pass --help to whatever binary we actually want the run script to wrap. Like I said though, I haven't tried ^this so I'm just tossing in whatever I just made up in my head.
+parser.add_argument('--selinux', + action='store_true', + help='Run in the appropriate selinux context') + +opts, args = parser.parse_known_args()
If you want to use ^this, then you need to be aware of prefix matching on the options recognized by Argparse. IOW if one is to pass <args> to the <binary> then none of the <args> can be a prefix of any of the long options argeparse knows about (in this case --selinux), otherwise it'll consume it. Altough unlikely, we should stay on the safe side and use: argparse.ArgumentParser(..., allow_abbrev=False) [1]
[2] https://docs.python.org/3.11/library/argparse.html?highlight=argparse#allow-...
ok, I wasn't aware of that option.
+ +if len(args) < 1: + print("syntax: %s [--selinux] BINARY [ARGS...]" % sys.argv[0], file=sys.stderr) sys.exit(1)
Same here, with argparse ^this is not needed if the args/options are defined correctly.
-prog = sys.argv[1] -args = sys.argv[1:] +prog = args[0]
argparse's parser obj has a 'prog' attribute [2].
I think that's the wrong 'prog' though. That would give me the './run' script, whereas I want the 'libvirtd' program (or whatever) that the user wants to execute.
Sorry, I completely overlooked the lack of 'sys.' in there, you're absolutely right. Erik
participants (2)
-
Erik Skultety -
Jonathon Jongsma