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

adding a method to patch linux idle check with required libraries. #5354

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

0x0OZ
Copy link

@0x0OZ 0x0OZ commented Sep 9, 2023

Fixes # Linux checking being idle, A fix for #1187

Description of the Change

added a method that does the check without really replacing it with the existing poor check to be reviewed and added by a C++ developer.
Alternate Designs

Release Notes

Fixing linux idle check by listening for input events in /dev/input/*.

If this change is not user-facing or notable enough to be included in release notes you may use the string "N/A" here. -->
To this run correctly, the user running the client has to have read and execute permissions on /dev/input/* files, this can be done using setfacl

}

// Main loop to monitor input events
while (true) {
Copy link
Member

Choose a reason for hiding this comment

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

This will lead to the dead lock of the BOINC Client. Instead, this functions should be called outside and return result as fast as possible.
Please be sure to test your changes before making a PR.

Copy link
Author

@0x0OZ 0x0OZ Sep 9, 2023

Choose a reason for hiding this comment

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

I want to have an opinion before making any further changes, This code could be added separately, runs in the background while the client is running, and writes the time of being idle to a tmp file, which can be read inside the client at any time.
But I am not sure if you agree with this idea to be added to the code,

Also, as I mentioned it will need permissions to read and execute /dev/input which regular users don't have by default.
So separating it could be a better approach to give the file its own permissions because having such permissions to a whole user is a security risk -keylogging is an example of what could go wrong-.

Copy link
Member

Choose a reason for hiding this comment

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

Don't get me wrong, but this is a not a good approach: you did some implementation, but you haven't tested it. What is more important - we can't test it as well, because we can't just use your code and run any test, because obviously everything will just hang in an infinite loop.
If you have an idea but don't know how to implement it properly - it's better to describe your idea in detail and ask for advice.

Copy link
Author

Choose a reason for hiding this comment

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

You are right.
I've removed the infinite loop and fixed the stupid logic.
But I also haven't removed any existing code.

I'll push the updates to the pull (when I figure out how).

Copy link
Member

Choose a reason for hiding this comment

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

While on this branch:
git add . && git commit && git push

// Use glob to enumerate input devices in /dev/input/
glob_t globbuf;
if (glob("/dev/input/event*", GLOB_NOSORT, nullptr, &globbuf) != 0) {
std::cerr << "Failed to enumerate input devices. " << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

msg_printfshould be used instead of the direct std:: calls.

Copy link
Member

Choose a reason for hiding this comment

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

Please fix this comment as well

Copy link
Author

Choose a reason for hiding this comment

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

It should be all good now.

@0x0OZ
Copy link
Author

0x0OZ commented Sep 10, 2023

It seems I added the includes in the wrong place as android is still trying to include them

https://github.com/BOINC/boinc/pull/5354/files#diff-b710fcc072350b8bbb1bc13eb394da5cdbc0b0dfab019a93890ec10eadb41b86R180-R188

@AenBleidd
Copy link
Member

@0x0OZ, Client is failed to build as well, you need to include 'libevdev' here to install it on CI:

sudo apt-get install -y libftgl-dev freeglut3-dev libcurl4-openssl-dev libxmu-dev libxi-dev libfcgi-dev libxss-dev libnotify-dev libxcb-util0-dev libgtk-3-dev libsecret-1-dev libgcrypt20-dev libsystemd-dev libwebkit2gtk-4.0-dev p7zip-full libxxf86vm-dev ocl-icd-opencl-dev zip

@0x0OZ
Copy link
Author

0x0OZ commented Sep 10, 2023

@AenBleidd client still fails,
I need to add -levdev flag to the MakeFile, but I am not sure how it's being built

@AenBleidd
Copy link
Member

@AenBleidd client still fails,

I need to add -levdev flag to the MakeFile, but I am not sure how it's being built

You need to add this linker flags somewhere to this file: https://github.com/BOINC/boinc/blob/master/configure.ac

@0x0OZ
Copy link
Author

0x0OZ commented Sep 10, 2023

@AenBleidd client still fails,
I need to add -levdev flag to the MakeFile, but I am not sure how it's being built

You need to add this linker flags somewhere to this file: https://github.com/BOINC/boinc/blob/master/configure.ac

Like I understand a single word 😅, I'll try to.

@0x0OZ
Copy link
Author

0x0OZ commented Sep 10, 2023

I made it worse with the last change to configure.ac...
I will try to find out how this thing work.

@0x0OZ
Copy link
Author

0x0OZ commented Sep 11, 2023

@AenBleidd Linux client now builds successfully,
But other clients still fail because they try to import libevdev when it should be included only in Linux env.

@0x0OZ
Copy link
Author

0x0OZ commented Sep 18, 2023

Hey @AenBleidd,
I want to remind you of this pull,
is there anything I need to do from my side?

@AenBleidd
Copy link
Member

@0x0OZ, it would be nice to fix this build for all other OSs...

@0x0OZ
Copy link
Author

0x0OZ commented Sep 19, 2023

Snap fails because libevdev is not being installed for it, it's only installed using apt, android fails because it tries to add the linker -levdev.
I thought adding the weird condition I added should fix it but it didn't, for snap I couldn't yet figure out where the package installation should be added.

@0x0OZ
Copy link
Author

0x0OZ commented Sep 20, 2023

@0x0OZ, it would be nice to fix this build for all other OSs...

That's done, everything builds successfully now.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #5354 (ffc5f0d) into master (eac3e3a) will decrease coverage by 0.01%.
Report is 16 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5354      +/-   ##
============================================
- Coverage     10.87%   10.87%   -0.01%     
  Complexity     1068     1068              
============================================
  Files           279      279              
  Lines         36059    36061       +2     
  Branches       8335     8335              
============================================
  Hits           3920     3920              
- Misses        31745    31747       +2     
  Partials        394      394              

see 3 files with indirect coverage changes

@0x0OZ
Copy link
Author

0x0OZ commented Sep 20, 2023

Snap failed, I thought it did not as at first it was skipped and didn't give an error.

@0x0OZ
Copy link
Author

0x0OZ commented Sep 21, 2023

@AenBleidd All success but snap, and I know what's wrong but I don't know what could be done because I can't reproduce the error and it's done in a container...
It seems_ that snap doesn't go over the libs path /usr/lib/x86_64-linux-gnu/, because apt installs the missing lib there.
but I don't know what causes this.
I installed an Ubuntu VM and tried Ubnutu container inside that VM, Both recognized libevdev.so.2.

@0x0OZ
Copy link
Author

0x0OZ commented Sep 28, 2023

I now have more time to work on this, Apart from the dead end for snap that I couldn't reproduce the bug and find a solution but it seems a docker thing? I have seen some issues that the libs installed during init weren't being recognized...
Also it doesn't detect ssh connection and it requires some privileges to access /dev/input/event* files.
because of this I think it would be better to make a stand-alone tool for an idle timer for every linux system.

This would be a better approach because of the permissions issue.

@AenBleidd
Copy link
Member

because of this I think it would be better to make a stand-alone tool for an idle timer for every linux system.

And that is why we have this idea: #5226.

Since you're already have some experience with that, maybe you could work on this tool?
IT would be also nice, if before starting a coding you could do an investigation first and present your solution in the ticket I just mentioned and afterwards start doing coding?

@0x0OZ
Copy link
Author

0x0OZ commented Sep 28, 2023

because of this I think it would be better to make a stand-alone tool for an idle timer for every linux system.

And that is why we have this idea: #5226.

Since you're already have some experience with that, maybe you could work on this tool? IT would be also nice, if before starting a coding you could do an investigation first and present your solution in the ticket I just mentioned and afterwards start doing coding?

Yes, I was thinking of doing that myself, I saw this ticket which gave me some insight.
After doing enough investigation I will share what I will build there which should solve the problem in all mentioned cases and be easily adaptable.

@AenBleidd
Copy link
Member

@0x0OZ, nice! Thank you very much! In case you have any questions - feel free to reach out to me here or in BOINC Discord: https://discord.gg/wPRafUq

@AenBleidd AenBleidd marked this pull request as draft October 7, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants