-
Notifications
You must be signed in to change notification settings - Fork 327
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
Improve error handling #1427
Improve error handling #1427
Conversation
@@ -68,12 +68,19 @@ namespace SDDM { | |||
if (env.value(QStringLiteral("XDG_SESSION_TYPE")) == QLatin1String("wayland") && env.value(QStringLiteral("XDG_SESSION_CLASS")) == QLatin1String("greeter")) { | |||
m_wayland = new WaylandHelper(this); | |||
m_wayland->setEnvironment(env); | |||
if (!m_wayland->startCompositor(m_displayServerCmd)) | |||
connect(m_wayland, &WaylandHelper::failed, this, &UserSession::stop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use a different path for late failure compared to failing to start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you're right that no-one seems to be calling stop()
Should this be called from:
Backend::closeSession
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use a different path for late failure compared to failing to start?
Which path are you referring to? ::setup calls exit() upon error
Should this be called from
Backend::closeSession
?
This will have closeSession be called because connect(m_session, &UserSession::finished, this, &HelperApp::sessionFinished);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to:
if (!m_wayland->startCompositor(m_displayServerCmd)) {
Q_EMIT finished(1);
and asking why this isn't
connect(m_wayland, &WaylandHelper::failed, this,[this]() {
Q_EMIT finished(1);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed by having stop() emit finished.
I don't think it makes sense to stop when we failed to start.
Be it because the x11 user session could not be started or because the wayland compositor can't.
The alternative is to stay stuck in a weird limbo
e522744
to
c8789d2
Compare
Now that we are not running sessions with almighty processes they're bound to fail.
This patch tries to increase the visibility of these problems and offers some mechanisms so that it doesn't leave the system stuck.