Mike Bird in bug 3025 reported missing initialization. Although the correction seems simple, there is one more thing to discuss.
The new_mask variable is used for the first time in sigprocmask on line 246. There is no initialization here, as Mike pointed out and as is fixed in proposed commit.
Until this point, everything was fine. However, then the new_mask variable is reused in the sigprocmask on the line 460. And here is the question of whether a mask set from line 426 should be used in this call or whether it should be reinitialized by sigfillset(…)? Because the comment mentions blocking all signals, just like in the first case.
Mike Bird in bug [3025](http://bugs.pearsoncomputing.net/show_bug.cgi?id=3025) reported missing initialization. Although the correction seems simple, there is one more thing to discuss.
The `new_mask` variable is used for the first time in [`sigprocmask` on line 246](src/branch/master/kdesktop/lock/main.cc#L246). There is no initialization here, as Mike pointed out and as is fixed in proposed commit.
Then `new_mask` variable is newly initialized using [`sigemptyset` on line 426](src/branch/master/kdesktop/lock/main.cc#L426) and subsequent calls `sigaddset` and then used in [`sigprocmask` on line 449](src/branch/master/kdesktop/lock/main.cc#L449).
Until this point, everything was fine. However, then the `new_mask` variable is reused in the [`sigprocmask` on the line 460](src/branch/master/kdesktop/lock/main.cc#L460). And here is the question of whether a mask set from line 426 should be used in this call or whether it should be reinitialized by `sigfillset(…)`? Because the comment mentions blocking **all** signals, just like in the first case.
SlavekB
added this to the R14.0.7 release milestone 5 years ago
I have checked the code too. It seems we need to initialize again new_mask with sigfillset() before the last call, if we want to block all signals as the comment says. Not sure what difference it will make, so we should test before merging.
I have checked the code too. It seems we need to initialize again new_mask with sigfillset() before the last call, if we want to block all signals as the comment says. Not sure what difference it will make, so we should test before merging.
Note: In a comment 5 on bugzilla I mentioned the important fact that in the stable branch used by Mike, calls sigprocmask are different and there is no call without initialization.
In any case, sigfillset(&new_mask); is probably also to be used for the stable branch as we discussed above.
Note: In a [comment 5](http://bugs.pearsoncomputing.net/show_bug.cgi?id=3025#c5) on bugzilla I mentioned the important fact that in the stable branch used by Mike, calls `sigprocmask` are different and there is no call without initialization.
In any case, `sigfillset(&new_mask);` is probably also to be used for the stable branch as we discussed above.
I have tested this PR in R14.1.0-dev and I see major issues with blocking all signals.
screensaver becomes unresponsive: moving or clicking the mouse does not show the unlock dialog immediately but need to wait for several seconds
after unlocking the screen, need to wait several seconds for the desktop to be useful
I repeated the tests on 2 different users and switching back-and-forth with the master version. Each time I tested the PR I faced the same problems, while using the master version there were no issues.
It seems blocking all signals is probably wrong, and thinking about it, it sort of make sense, since we are also blocking the response to IO events such as the mouse. In fact I now wonder how it could still work with all the signal blocked 😧
I have tested this PR in R14.1.0-dev and I see major issues with blocking all signals.
1. screensaver becomes unresponsive: moving or clicking the mouse does not show the unlock dialog immediately but need to wait for several seconds
2. after unlocking the screen, need to wait several seconds for the desktop to be useful
I repeated the tests on 2 different users and switching back-and-forth with the master version. Each time I tested the PR I faced the same problems, while using the master version there were no issues.
It seems blocking all signals is probably wrong, and thinking about it, it sort of make sense, since we are also blocking the response to IO events such as the mouse. In fact I now wonder how it could still work with all the signal blocked :anguished:
Yes, I can confirm that the behavior on R14.0.x is also significantly worse when all signals are blocked. The unlock window has a long delay and the screen saver is not properly deleted. When the unlock window is closed (without unlock), the screen saver is not restarted.
Note: Only the second part of the patch is applied because the first is not valid for r14.0.x branch.
Yes, I can confirm that the behavior on R14.0.x is also significantly worse when all signals are blocked. The unlock window has a long delay and the screen saver is not properly deleted. When the unlock window is closed (without unlock), the screen saver is not restarted.
Note: Only the second part of the patch is applied because the first is not valid for r14.0.x branch.
SlavekB
changed title from kdesktop: Add missing initialization of sigset_t in kdesktop_lock. to WIP: kdesktop: Add missing initialization of sigset_t in kdesktop_lock.5 years ago
I have updated Slavek's commit removing the last sigfillset() call. Tested on R14.1.0-dev, seems to work fine.
I have updated Slavek's commit removing the last sigfillset() call. Tested on R14.1.0-dev, seems to work fine.
MicheleC
changed title from WIP: kdesktop: Add missing initialization of sigset_t in kdesktop_lock. to kdesktop: Add missing initialization of sigset_t in kdesktop_lock.5 years ago
Mike Bird in bug 3025 reported missing initialization. Although the correction seems simple, there is one more thing to discuss.
The
new_mask
variable is used for the first time insigprocmask
on line 246. There is no initialization here, as Mike pointed out and as is fixed in proposed commit.Then
new_mask
variable is newly initialized usingsigemptyset
on line 426 and subsequent callssigaddset
and then used insigprocmask
on line 449.Until this point, everything was fine. However, then the
new_mask
variable is reused in thesigprocmask
on the line 460. And here is the question of whether a mask set from line 426 should be used in this call or whether it should be reinitialized bysigfillset(…)
? Because the comment mentions blocking all signals, just like in the first case.I have checked the code too. It seems we need to initialize again new_mask with sigfillset() before the last call, if we want to block all signals as the comment says. Not sure what difference it will make, so we should test before merging.
Note: In a comment 5 on bugzilla I mentioned the important fact that in the stable branch used by Mike, calls
sigprocmask
are different and there is no call without initialization.In any case,
sigfillset(&new_mask);
is probably also to be used for the stable branch as we discussed above.I have tested this PR in R14.1.0-dev and I see major issues with blocking all signals.
screensaver becomes unresponsive: moving or clicking the mouse does not show the unlock dialog immediately but need to wait for several seconds
after unlocking the screen, need to wait several seconds for the desktop to be useful
I repeated the tests on 2 different users and switching back-and-forth with the master version. Each time I tested the PR I faced the same problems, while using the master version there were no issues.
It seems blocking all signals is probably wrong, and thinking about it, it sort of make sense, since we are also blocking the response to IO events such as the mouse. In fact I now wonder how it could still work with all the signal blocked 😧
Yes, I can confirm that the behavior on R14.0.x is also significantly worse when all signals are blocked. The unlock window has a long delay and the screen saver is not properly deleted. When the unlock window is closed (without unlock), the screen saver is not restarted.
Note: Only the second part of the patch is applied because the first is not valid for r14.0.x branch.
kdesktop: Add missing initialization of sigset_t in kdesktop_lock.to WIP: kdesktop: Add missing initialization of sigset_t in kdesktop_lock. 5 years agoI have updated Slavek's commit removing the last sigfillset() call. Tested on R14.1.0-dev, seems to work fine.
WIP: kdesktop: Add missing initialization of sigset_t in kdesktop_lock.to kdesktop: Add missing initialization of sigset_t in kdesktop_lock. 5 years agoChanged to R14.1.0 milestone, since the changes in this PR are only valid for the master branch.
e47bab3fd8
.