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

Proposed fix for swayidle forking behaviour #3382

Closed
wants to merge 4 commits into from

Conversation

ianyfan
Copy link
Contributor

@ianyfan ianyfan commented Jan 7, 2019

This should fix #3119, but I'm not sure whether it has the potential to leave zombie processes.

swayidle/main.c Outdated
} else {
// sd_bus_message_unref closes the file descriptor so we need
Copy link
Member

@emersion emersion Jan 7, 2019

Choose a reason for hiding this comment

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

Style: can we replace this with goto-based error handling?

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Since we waitpid we won't leave zombies behind. I think the default config has some swayidle example that needs to be updated. Do we want -f to be the default?

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

In any case, we should remove the 1 second timer.

@ianyfan
Copy link
Contributor Author

ianyfan commented Jan 8, 2019

I don't think forking should be the default, but if you want it to be, please suggest what the flag and description should be.

@emersion
Copy link
Member

emersion commented Jan 8, 2019

I think -f is confusing. swaylock -f daemonizes swaylock (ie. swaylock starts in the background). Here, the behaviour is different: swayidle either waits for child processes to finish, or runs them in the background.

@ianyfan
Copy link
Contributor Author

ianyfan commented Jan 8, 2019

Makes sense. I've changed it to -w to wait for the command to finish executing. What do you think?

@emersion
Copy link
Member

emersion commented Jan 8, 2019

This looks good. I think the example config still needs to be updated, to use swayidle -w and swaylock -f?

@emersion
Copy link
Member

emersion commented Jan 8, 2019

We also need to make sure this correctly locks the screen before idling.

}

// sd_bus_message_unref closes the file descriptor so we need
// to copy it beforehand
lock_fd = fcntl(lock_fd, F_DUPFD_CLOEXEC, 3);
if (lock_fd < 0) {
wlr_log(WLR_ERROR, "Failed to copy sleep lock fd: %s",
strerror(errno));
wlr_log(WLR_ERROR, "Failed to copy sleep lock fd: %s", strerror(errno));
Copy link
Member

Choose a reason for hiding this comment

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

Missing goto here

@@ -16,6 +16,10 @@ swayidle - Idle manager for Wayland
*-d*
Enable debug output.

*-w*
Wait for command to finish executing before continuing, helpful for ensuring
that a *before-sleep* command has finished before the system goes to sleep.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a warning like "swayidle will block until the command finishes".

@ianyfan
Copy link
Contributor Author

ianyfan commented Jan 13, 2019

This needs to be moved to the new repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

After resuming from suspend: desktop briefly shows up before lockscreen
2 participants