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

Revise scrotWaitUntil interface #323

Merged
merged 4 commits into from
May 31, 2023

Conversation

N-R-K
Copy link
Collaborator

@N-R-K N-R-K commented May 26, 2023

The timespec argument for nanosleep is awkward because it uses ns when we want to sleep for some ms instead. And scrotWaitUntil is awkward because it uses absolute time, instead of relative time.

Add scrotWaitFor which uses a relative time in ms and uses scrotWaitUntil internally.

CC: @guijan

src/scrot.h Outdated Show resolved Hide resolved
src/scrot.c Outdated Show resolved Hide resolved
src/scrot.c Outdated Show resolved Hide resolved
src/scrot.c Outdated Show resolved Hide resolved
@guijan
Copy link
Contributor

guijan commented May 26, 2023

This API could probably replace scrotWaitUntil() at a later point, would be less lines of code with no difference in behavior.

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 26, 2023

This API could probably replace scrotWaitUntil() at a later point, would be less lines of code with no difference in behavior.

Hmm, hadn't thought about it but you're probably right. I'll try to do this tomorrow with a fresher mind.

@N-R-K N-R-K marked this pull request as draft May 26, 2023 18:57
@N-R-K N-R-K marked this pull request as draft May 26, 2023 18:57
N-R-K added 3 commits May 27, 2023 11:34
this will be useful for converting some of the nanosleep calls into
scrotSleepFor().
absolute time is really awkward to use. scrotSleepFor() instead takes a
starting timestamp and a relative millisecond offset from that start and
returns back the ending timestamp.

this, combined with clockNow(), makes the API easy to use and easy to
replace the existing nanosleep calls with.
@N-R-K N-R-K marked this pull request as ready for review May 27, 2023 06:22
@N-R-K N-R-K changed the title Use scrotWaitFor instead of nanosleep Revise scrotWaitUntil interface May 27, 2023
if (tmp.tv_nsec < 0) {
tmp.tv_sec--;
tmp.tv_nsec += 1000000000L;
--tmp.tv_sec;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The webkit style guide doesn't say anything about preference toward pre vs post increments. And Ctrl+F "++" reveals many examples with both being used.

* correct for drift and call nanosleep() everywhere.
*/
struct timespec ret;
#if defined(__linux__)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't think the CONTINUOUS_CLOCK needed to be "public" anymore - since you can just use clockNow() instead. So the ifdef can be done internally for clockNow.

(Though the main reason I added this function was to work around the awkwardness of clock_gettime's outparam, e.g scrotSleepFor(clockNow(), 64) is much easier to use.)

@guijan
Copy link
Contributor

guijan commented May 30, 2023

Hmm, using the interface as is to replace scrotWaitUntil() means that with 32-bit int, we can wait a maximum of 24 days to take the screenshot. I wonder if that's too close for comfort?
Making that into long long doesn't help because then it assumes time_t is 64-bit which isn't necessarily the case on i386 glibc.

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 30, 2023

we can wait a maximum of 24 days to take the screenshot. I wonder if that's too close for comfort?

opt.delay is limited to INT_MAX. If someone needs more, they can use sleep 69d && scrot or something along those line.

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 30, 2023

Ah, wait. opt.delay is in seconds, but this function uses ms. Should also reduce opt.delay to INT_MAX/1000 to avoid mul overflow.

@guijan
Copy link
Contributor

guijan commented May 31, 2023

If you provide a version of the function that takes a timespec, and then implement the one that uses milliseconds by calling the timespec version, we don't need the 24 day limit on scrot --delay. I like pushing limits high, we just did that with filenames, but now the delay limit is falling.

@guijan
Copy link
Contributor

guijan commented May 31, 2023

Also, with 32-bit timespec (i.e. i386 glibc), it's possible to specify an int large enough to make struct timespec's tv_sec member overflow on master, if you write a timespec version, check for overflow.

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 31, 2023

I like pushing limits high, we just did that with filenames, but now the delay limit is falling.

I think that energy is better spent on issues like #320 or feature requests like #82 that actually concerns scrot's primary purpose - to take screenshots. There's also too many other low hanging fruits with more practical issues (e.g #228) to be bothering with impractical things like 24 day+ delay.

(Honestly this entire sleep saga to me seems like wasted effort fixing a non-issue that would've affected no-one.)

avoids potential multiplication overflow when the seconds are converted
into milliseconds.
@N-R-K N-R-K merged commit 3c89049 into resurrecting-open-source-projects:master May 31, 2023
@N-R-K N-R-K deleted the waitFor branch May 31, 2023 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants