Hi,
I don't know how closely the devs watch the Bug list but I believe I've fixed Bug 2998.
As this bug results in kdesktop/kdesktop_lock eating CPU and going unresponsive; might I humbly request that the fix is incorporated in the next possible/dev version.
As the code I posted on bugzilla used a crude fixed buffer, here's a cleaned up version that uses malloc.
*** /tmp/tdebase-trinity-14.0.6~pre38/kdesktop/lock/main.cc Thu Feb 7 15:48:21 2019 --- /usr/tmp/tdebase-trinity-14.0.6~pre38/kdesktop/lock/main.cc Sun May 20 19:41:55 2018 *************** *** 325,336 **** #endif }
! char *locknameroot="kdesktop_lock_lockfile."; ! char *lockfilename = (char*)malloc(strlen(locknameroot) + strlen(getenv("DISPLAY")) + 1); ! strcpy(lockfilename,locknameroot); ! strcat(lockfilename,getenv("DISPLAY")); ! ! TDELockFile lock(locateLocal("tmp", lockfilename)); lock.setStaleTime(0); TDELockFile::LockResult lockRet = lock.lock(); if (lockRet != TDELockFile::LockOK) { --- 325,331 ---- #endif }
! TDELockFile lock(locateLocal("tmp", "kdesktop_lock_lockfile")); lock.setStaleTime(0); TDELockFile::LockResult lockRet = lock.lock(); if (lockRet != TDELockFile::LockOK) {
On 02/07/2019 09:52 AM, Russell Brown wrote:
As the code I posted on bugzilla used a crude fixed buffer, here's a cleaned up version that uses malloc.
*** /tmp/tdebase-trinity-14.0.6~pre38/kdesktop/lock/main.cc Thu Feb 7 15:48:21 2019 --- /usr/tmp/tdebase-trinity-14.0.6~pre38/kdesktop/lock/main.cc Sun May 20 19:41:55 2018
*** 325,336 **** #endif }
! char *locknameroot="kdesktop_lock_lockfile."; ! char *lockfilename = (char*)malloc(strlen(locknameroot) + strlen(getenv("DISPLAY")) + 1);
I haven't followed the issue too closely, but in the past, there was an issue that would cause high CPU usage caused by a security addition to either KDM or the lockscreen itself. I would ensure this wasn't a regression (Slavek may recall, it was in the 2012-14 timeframe)
Regarding the proposed code, only one quandary -- where is lockfilename freed? (if this is allocated in a forked process that later dies and releases the memory then you are fine, but if not, it seems this would leak memory each time lockfilename is allocated -- unless there is a free later not shown in the code posted)
On Thursday 07 of February 2019 16:52:21 Russell Brown wrote:
Hi,
I don't know how closely the devs watch the Bug list but I believe I've fixed Bug 2998.
As this bug results in kdesktop/kdesktop_lock eating CPU and going unresponsive; might I humbly request that the fix is incorporated in the next possible/dev version.
As the code I posted on bugzilla used a crude fixed buffer, here's a cleaned up version that uses malloc.
*** /tmp/tdebase-trinity-14.0.6~pre38/kdesktop/lock/main.cc Thu Feb 7 15:48:21 2019 --- /usr/tmp/tdebase-trinity-14.0.6~pre38/kdesktop/lock/main.cc Sun May 20 19:41:55 2018 *************** *** 325,336 **** #endif }
! char *locknameroot="kdesktop_lock_lockfile."; ! char *lockfilename = (char*)malloc(strlen(locknameroot) + strlen(getenv("DISPLAY")) + 1); ! strcpy(lockfilename,locknameroot); ! strcat(lockfilename,getenv("DISPLAY")); ! ! TDELockFile lock(locateLocal("tmp", lockfilename)); lock.setStaleTime(0); TDELockFile::LockResult lockRet = lock.lock(); if (lockRet != TDELockFile::LockOK) { --- 325,331 ---- #endif }
! TDELockFile lock(locateLocal("tmp", "kdesktop_lock_lockfile")); lock.setStaleTime(0); TDELockFile::LockResult lockRet = lock.lock(); if (lockRet != TDELockFile::LockOK) {
Hi Russell,
your observation gives a very clear sense. The name of the lock does not distinguish between $DISPLAY and therefore two separate kdesktop_lock processes struggles for one file. Good conclusion!
Because locateLocal expects a TQString value as argument, I suggest using this fact and make the patch simpler:
--- a/kdesktop/lock/main.cc +++ b/kdesktop/lock/main.cc @@ -325,7 +325,7 @@ #endif }
- TDELockFile lock(locateLocal("tmp", "kdesktop_lock_lockfile")); + TDELockFile lock(locateLocal("tmp", TQString("kdesktop_lock_lockfile.%1").arg(getenv("DISPLAY")))); lock.setStaleTime(0); TDELockFile::LockResult lockRet = lock.lock(); if (lockRet != TDELockFile::LockOK) {
If you want to get involved, the best way is to register on TDE Gitea Workspace and create a pull-request. See:
https://wiki.trinitydesktop.org/TDE_Gitea_Workspace
Note: Pull-requests should always be on the master branch. Core developers then take care of a backport patch into a stable branch.
Note1: By the way, in your proposed code is missing free, so this would leads to memory leaks.
Cheers
Hi,
Note that this is essentially a lighter version of the issue in bug 1766.
I'm digging it right now, but a simple adding of the DISPLAY won't fix it for me because as for now there are actually 2 instances for each of two screens (4 in total, see the bug for details) are fighting over control. Having a squared-number-of-screen kdesktop_lock running all the time just looks messed up. Also they conflict with each other when locking a desktop from a «wrong screen».
So, I'd rather get rid of all of those constantly running kdesktop_lock(s) and leave only one or one-for-each-screen at most. But it requires some thine and gentle tweaking to achieve it and not compromise the security/stability.
I've tried to rearrange things in the `main()` at first (to lock and wait for a signal from kdesktop before forking), but it didn't gone specifically well, actually I've got caught up into some sort of cabbage-goat-wolf puzzle but with a flesh-eating cabbage this time...
As for now I'm looking forward for some changes in kdesktop/kdesktop_lock or inter-kdesktop communications. Also I was going to commit some refactoring on the kdesktop/lock/main.cc because as for now that mess is unmodifiable (on rather high level)...
чт, 7 февр. 2019 г. в 22:41, Slávek Banko slavek.banko@axis.cz:
On Thursday 07 of February 2019 16:52:21 Russell Brown wrote:
Hi,
I don't know how closely the devs watch the Bug list but I believe I've fixed Bug 2998.
As this bug results in kdesktop/kdesktop_lock eating CPU and going unresponsive; might I humbly request that the fix is incorporated in the next possible/dev version.
As the code I posted on bugzilla used a crude fixed buffer, here's a cleaned up version that uses malloc.
*** /tmp/tdebase-trinity-14.0.6~pre38/kdesktop/lock/main.cc Thu Feb 7 15:48:21 2019 --- /usr/tmp/tdebase-trinity-14.0.6~pre38/kdesktop/lock/main.cc Sun May 20 19:41:55 2018 *************** *** 325,336 **** #endif }
! char *locknameroot="kdesktop_lock_lockfile."; ! char *lockfilename = (char*)malloc(strlen(locknameroot) + strlen(getenv("DISPLAY")) + 1); ! strcpy(lockfilename,locknameroot); ! strcat(lockfilename,getenv("DISPLAY")); ! ! TDELockFile lock(locateLocal("tmp", lockfilename)); lock.setStaleTime(0); TDELockFile::LockResult lockRet = lock.lock(); if (lockRet != TDELockFile::LockOK) { --- 325,331 ---- #endif }
! TDELockFile lock(locateLocal("tmp", "kdesktop_lock_lockfile")); lock.setStaleTime(0); TDELockFile::LockResult lockRet = lock.lock(); if (lockRet != TDELockFile::LockOK) {
Hi Russell,
your observation gives a very clear sense. The name of the lock does not distinguish between $DISPLAY and therefore two separate kdesktop_lock processes struggles for one file. Good conclusion!
Because locateLocal expects a TQString value as argument, I suggest using this fact and make the patch simpler:
--- a/kdesktop/lock/main.cc +++ b/kdesktop/lock/main.cc @@ -325,7 +325,7 @@ #endif }
TDELockFile lock(locateLocal("tmp", "kdesktop_lock_lockfile"));
TDELockFile lock(locateLocal("tmp", TQString("kdesktop_lock_lockfile.%1").arg(getenv("DISPLAY")))); lock.setStaleTime(0); TDELockFile::LockResult lockRet = lock.lock(); if (lockRet != TDELockFile::LockOK) {If you want to get involved, the best way is to register on TDE Gitea Workspace and create a pull-request. See:
https://wiki.trinitydesktop.org/TDE_Gitea_Workspace
Note: Pull-requests should always be on the master branch. Core developers then take care of a backport patch into a stable branch.
Note1: By the way, in your proposed code is missing free, so this would leads to memory leaks.
Cheers
Slávek
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
I'm digging it right now, but a simple adding of the DISPLAY won't fix it for me because as for now there are actually 2 instances for each of two screens (4 in total, see the bug for details) are fighting over control. Having a squared-number-of-screen kdesktop_lock running all the time just looks messed up. Also they conflict with each other when locking a desktop from a «wrong screen».
As this bug results in kdesktop/kdesktop_lock eating CPU and going
unresponsive; might I humbly request that the fix is incorporated in the next possible/dev version.
I have not yet looked into the code in detail, but the kdesktop_lock is responsible for locking the screen. AFAICR from previous work on tdm+locking, kdesktop_lock use one file to determine the active lock process and eventually kill stale processes before doing his things. If we have multiple screens, we can not have multiple kdesktop_lock associated with each screen, for the simple reason that we can not have a screen locked and the other not locked. If we have multiple *different* user sessions, the lock files will live in different folders (again AFAICR) and they should not conflict. If we have multiple sessions from *the same* user, the different lock processes will conflict over the lock file and this will probably cause issues.
just my 2 cents, until I can find the time to look at this in more details.
Cheers Michele
Hi Fat-Zer,
although it is clear that Russell's patch will not be able to solve all of the problems mentioned in 1766, it is a good step forward - quickly and simply solving one particular problem. Therefore, there is good reason to merge it.
Cheers
сб, 9 февр. 2019 г. в 21:41, Slávek Banko slavek.banko@axis.cz:
Hi Fat-Zer,
although it is clear that Russell's patch will not be able to solve all of the problems mentioned in 1766, it is a good step forward - quickly and simply solving one particular problem. Therefore, there is good reason to merge it.
Cheers
Sure, go ahead.
Quoth Sl�vek Banko.....
Because locateLocal expects a TQString value as argument, I suggest using this fact and make the patch simpler:
TDELockFile lock(locateLocal("tmp", "kdesktop_lock_lockfile"));
TDELockFile lock(locateLocal("tmp", TQString("kdesktop_lock_lockfile.%1").arg(getenv("DISPLAY"))));
Thank you Sl�vek, I thought there would be a TDE/TQT way of doing this.
I'll change my in-use version to this.
On Friday 08 of February 2019 10:17:11 Russell Brown wrote:
Quoth Slávek Banko.....
Because locateLocal expects a TQString value as argument, I suggest using this fact and make the patch simpler:
TDELockFile lock(locateLocal("tmp","kdesktop_lock_lockfile")); + TDELockFile lock(locateLocal("tmp", TQString("kdesktop_lock_lockfile.%1").arg(getenv("DISPLAY"))));
Thank you Slávek, I thought there would be a TDE/TQT way of doing this.
I'll change my in-use version to this.
Hi Russell,
what next step will be? Are you interested in joining TGW and entering pull request? Or do I push a patch?
Cheers
Hi Sl�vek,
Quoth Sl�vek Banko.....
Because locateLocal expects a TQString value as argument, I suggest using this fact and make the patch simpler:
TDELockFile lock(locateLocal("tmp","kdesktop_lock_lockfile")); + TDELockFile lock(locateLocal("tmp", TQString("kdesktop_lock_lockfile.%1").arg(getenv("DISPLAY"))));
what next step will be? Are you interested in joining TGW and entering pull request? Or do I push a patch?
Can you do the honours and push the patch.
Thanks.
-
David C. Rankin -
Fat-Zer -
Michele Calgaro via trinity-devel -
Russell Brown -
Slávek Banko