BunsenLabs / bunsen-utilities

https://pkg.bunsenlabs.org/debian/pool/main/b/bunsen-utilities/
GNU General Public License v3.0
30 stars 21 forks source link

bl-exit --logout fails #24

Closed johnraff closed 8 years ago

johnraff commented 8 years ago

Since os.system was replaced the logout option no longer works:

~$ bl-exit -l
Traceback (most recent call last):
  File "/usr/bin/bl-exit", line 151, in <module>
    main(sys.argv[1:])
  File "/usr/bin/bl-exit", line 135, in main
    logout()
  File "/usr/bin/bl-exit", line 105, in logout
    os.system("openbox --exit")
NameError: global name 'os' is not defined

On line 105, replacing os.system("openbox --exit") with call("openbox --exit") produced:

Execution failed:  [Errno 2] No such file or directory
Hjdskes commented 8 years ago

From the subprocess.call documentation:

args is required for all calls and should be a string, or a sequence of program arguments. Providing a sequence of arguments is generally preferred, as it allows the module to take care of any required escaping and quoting of arguments (e.g. to permit spaces in file names). If passing a single string, either shell must be True (see below) or else the string must simply name the program to be executed without specifying any arguments.

Indeed, using variable arguments in our call function and replacing os.system("openbox --exit") (which I apparantly glanced over :expressionless:) with call("openbox", "--exit") works. The strange thing is that the send_dbus function does not send a sequence of program arguments but does work, while call("openbox --exit") clearly doesn't!

I'm going to look into it some more, but here's a patch that fixes the problem:

diff --git a/bl-exit b/bl-exit
index c6478f4..e8595d6 100755
--- a/bl-exit
+++ b/bl-exit
@@ -91,9 +91,9 @@ class bl_exit(gtk.Window):
         self._status.set_label("Shutting down, please standby...")
         poweroff()

-def call(command):
+def call(*args):
     try:
-        subprocess.call(command)
+        subprocess.call(args)
     except OSError as e:
         print >>sys.stderr, "Execution failed: ", e

@@ -102,7 +102,7 @@ def send_dbus(string):
     call(dbus_send.format(string))

 def logout():
-    os.system("openbox --exit")
+    call("openbox", "--exit")

 def suspend():
     call("bl-lock")
Hjdskes commented 8 years ago

Using shell=True in subprocess.call() for the logout command also fixes the issue. I'm baffled why this is needed for openbox --exit and not dbus-send --print-reply --system ....

@2ion I know you have enough work on your plate at the moment, but perhaps you know what is going on here? No hurries :blush:


@johnraff The issue I'm looking into now is purely out of interest. If you want to resolve this issue, feel free to apply the above diff or ask me to send in a pull request (whichever has your preference). If you want consistency, here's a diff that also breaks up the dbus_send string:

diff --git a/bl-exit b/bl-exit
index c6478f4..0ee6197 100755
--- a/bl-exit
+++ b/bl-exit
@@ -91,18 +91,19 @@ class bl_exit(gtk.Window):
         self._status.set_label("Shutting down, please standby...")
         poweroff()

-def call(command):
+def call(*args):
     try:
-        subprocess.call(command)
+        subprocess.call(args)
     except OSError as e:
         print >>sys.stderr, "Execution failed: ", e

 def send_dbus(string):
-    dbus_send = "dbus-send --print-reply --system --dest=org.freedesktop.login1 /org/freedesktop/login1 org.freedesktop.login1.Manager.{} boolean:true"
-    call(dbus_send.format(string))
+    call("dbus-send", "--print-reply", "--system", "--dest=org.freedesktop.login1",
+            "/org/freedesktop/login1", "org.freedesktop.login1.Manager.{}".format(string),
+            "boolean:true")

 def logout():
-    os.system("openbox --exit")
+    call("openbox", "--exit")

 def suspend():
     call("bl-lock")
ghost commented 8 years ago

@Unia The shell applies the IFS to the input line in order to break it into tokens, leading to the first 'word' of "openbox --exit" being recognized as the 'command' $0 and the rest as $1 [..$n]. Shell=Fast means that the line won't get tokenized before getting passed to execvp() (or the like) as the first argument, and given that no executable named "openbox --exit" exists, the call fails.

Hjdskes commented 8 years ago

I understand that part, but why then does it work for the dbus_send string?

Hjdskes commented 8 years ago

It seems like there is something going very wrong at my system. I reverted my changes (the second diff in this thread) and now I get the same exception. Just this morning when I tested it, my system did suspend when pressing the suspend button, so I have no idea really what is going on here.

Anyway, @johnraff, I'll send in a PR now applying the second diff.