Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(exec): redirect app output to /dev/null #9411

Merged
merged 1 commit into from
Feb 15, 2025
Merged

Conversation

nnyyxxxx
Copy link
Contributor

fixes #9278

this is a better approach to #1000 and yes waybar opens lol unlike #1013

@nnyyxxxx
Copy link
Contributor Author

image

left is after, right is before

@PaideiaDilemma
Copy link
Contributor

PaideiaDilemma commented Feb 15, 2025

Also it is the same as #1000 right?
Both MRs leave STDIN open. Is there a reason why? It still points to the tty I think.

I also looked into this (and generally process spawning)
Tried to figure out why #1000 originally caused problem without success. Whatever caused waybar not to start, has probably been fixed.
I think std* pointing to /dev/null should not cause any problems.

@nnyyxxxx
Copy link
Contributor Author

i have tried before and after, this one fixes the problem where waybar does not spawn, the other one still has the same problem.

not the same

not sure why the original one causes the issue either tbh

@nnyyxxxx
Copy link
Contributor Author

heres an updated version of #1000 if you want to test for yourself

diff.txt

@PaideiaDilemma
Copy link
Contributor

I used this on latest commit

diff --git a/src/managers/KeybindManager.cpp b/src/managers/KeybindManager.cpp
index 8daa6838..768f3198 100644
--- a/src/managers/KeybindManager.cpp
+++ b/src/managers/KeybindManager.cpp
@@ -24,6 +24,8 @@
 #include <string>
 #include <string_view>
 #include <cstring>
+#include <fcntl.h>
+#include <paths.h>
 
 #include <hyprutils/string/String.hpp>
 #include <hyprutils/os/FileDescriptor.hpp>
@@ -967,6 +969,22 @@ uint64_t CKeybindManager::spawnRawProc(std::string args, PHLWORKSPACE pInitialWo
                 setenv(e.first.c_str(), e.second.c_str(), 1);
             }
             setenv("WAYLAND_DISPLAY", g_pCompositor->m_szWLDisplaySocket.c_str(), 1);
+            close(STDOUT_FILENO);
+            close(STDERR_FILENO);
+
+            int devnull = open(_PATH_DEVNULL, O_WRONLY);
+            if (devnull == -1) {
+                Debug::log(LOG, "Unable to open /dev/null for writing");
+            }
+
+            if (dup2(devnull, STDOUT_FILENO) == -1) {
+                Debug::log(LOG, "Unable to duplicate /dev/null to stdout");
+            }
+            if (dup2(devnull, STDERR_FILENO) == -1) {
+                Debug::log(LOG, "Unable to duplicate /dev/null to stderr");
+            }
+
+            close(devnull);
             execl("/bin/sh", "/bin/sh", "-c", args.c_str(), nullptr);
             // exit grandchild
             _exit(0);

And waybar opens just fine ???

@PaideiaDilemma
Copy link
Contributor

PaideiaDilemma commented Feb 15, 2025

Shouldn't they be functionally equivalent?

Maybe opening devnull can fail??

@nnyyxxxx
Copy link
Contributor Author

i think the issue is that it does not open with an exec-once, not the fact that it doesnt open in general

@nnyyxxxx
Copy link
Contributor Author

could you clarify which way you are talking about? ^^

@PaideiaDilemma
Copy link
Contributor

shi* I forgot that I wrapped waybar in uwsm 🙃
Ok yeah I can repo and how the f does your patch work xDDD

@nnyyxxxx
Copy link
Contributor Author

skillz

@PaideiaDilemma
Copy link
Contributor

is it _PATH_DEVNULL?

@nnyyxxxx
Copy link
Contributor Author

nope

@PaideiaDilemma
Copy link
Contributor

It's actually the closing of the stdin and stdout that breaks it.
That is pretty weird.

@PaideiaDilemma
Copy link
Contributor

But if it works it works I guess. And stdin? Is it best to just leave it pointing to the tty/pts?

@nnyyxxxx
Copy link
Contributor Author

ye

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel alive

@vaxerski vaxerski merged commit 7a6fde8 into hyprwm:main Feb 15, 2025
12 checks passed
nnyyxxxx added a commit to nnyyxxxx/Hyprland that referenced this pull request Feb 15, 2025
andrewandreii pushed a commit to andrewandreii/Hyprland that referenced this pull request Feb 15, 2025
nnyyxxxx added a commit to nnyyxxxx/Hyprland that referenced this pull request Feb 15, 2025
@nnyyxxxx nnyyxxxx deleted the patch-3 branch February 15, 2025 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After exit I get Error if I left a application open
3 participants