Skip to content

client (Unix): add support for idle detection in a separate process #6242

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

davidpanderson
Copy link
Contributor

@Copilot Copilot AI review requested due to automatic review settings April 11, 2025 21:09
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment on lines 2212 to 2213
// (big mess, permssions problems, doesn't generally work)
// new approach: get last-input time from a separate daemon process,
Copy link
Preview

Copilot AI Apr 11, 2025

Choose a reason for hiding this comment

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

Typo in the comment: 'permssions' should be corrected to 'permissions'.

Suggested change
// (big mess, permssions problems, doesn't generally work)
// new approach: get last-input time from a separate daemon process,
// (big mess, permissions problems, doesn't generally work)

Copilot uses AI. Check for mistakes.


uint64_t input_time = seg_ptr[1];
idle_time = gstate.now - input_time;
printf("idle time: %ld\n", idle_time);
Copy link
Preview

Copilot AI Apr 11, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using the project's logging mechanism instead of printf for production logging to ensure consistency and proper log level management.

Copilot uses AI. Check for mistakes.

…ction.

shm_open() and mmap() are the standard way to do this.

This requires -lrt.
I just added this to Makefile.am; probably not the right way.
@jamescowens
Copy link

jamescowens commented Apr 12, 2025

@davidpanderson do you mind making these signed 64 bit ints instead of unsigned so that we conform to the new time standard?

POSIX requires time to be a signed int. The complete spec in <time.h> is actually

struct timespec {
    time_t   tv_sec;        /* seconds */
    long     tv_nsec;       /* nanoseconds */
};

Where time_t is typedefd internally in the header file to a 64 bit signed integer type on that platform.

We obviously don't need the nanoseconds part for this so just using the 64 bit signed integer is perfectly fine.

@davidpanderson
Copy link
Contributor Author

Uh, sure.

@AenBleidd
Copy link
Member

This PR fails during the build for Android:

hostinfo_unix.cpp:567:52: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
            strlcpy(buf2, strchr(buf, ':') + 2, ((t<sizeof(buf2))?t:sizeof(buf2)));
                                                  ~^~~~~~~~~~~~~
hostinfo_unix.cpp:2219:18: error: use of undeclared identifier 'shm_open'
        int fd = shm_open("/idle_detect_shmem",  O_RDONLY, 0);
                 ^
1 warning and 1 error generated.

@davidpanderson
Copy link
Contributor Author

fixed (I hope)

- include -lcrypto after -lssl
- include -lrt only if it exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment