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

Documentation for qb_loop_timer_expire_time_get seems misleading #390

Closed
cmurphycode opened this issue Apr 20, 2020 · 2 comments
Closed

Comments

@cmurphycode
Copy link
Contributor

When a timer is registered using timerlist_add_duration, the field expire_time is set to the current time plus a user-requested amount of time. That value is plainly returned by timerlist_expire_time.

The only caller of timerlist_expire_time is qb_loop_timer_expire_time_get.

The function documentation reads as follows:

* Get the time remaining before it expires.

Which, to me, implies that the time value returned is not an absolute time, but rather the amount left in the user specified duration. That is, roughly, expire_time - current_time().

I would have filed this with a pull request to fix it, however, that would surely break any existing caller who has already worked around this. I noticed that currently, qb_loop_timer_expire_time_get is not used by anything in libqb* or corosync, but it is exposed by apidef_corosync_api_v1, so I'm guessing we don't want to change it.
NOTE: it is used by qb_loop_timer_is_running, but only the part of the behavior which returns 0 if it's expired, which we could keep.

Maybe the best thing to do is to fix the comment and then specify a new function?

Any thoughts are welcome. If it helps, I would be happy to contribute a patch with whichever direction you see fit.

Thank you!

@chrissie-c
Copy link
Contributor

Thanks for pointing this out! I think it would be wrong to change the behaviour of the call as it stands - that would be an ABI change. So I think what needs to be done is to fix the documentation and optionally add a new call to do it properly. As I'm slowly building up to a 2.0 release (don't hold your breath) now is a good time to add that new call if you want :)

@chrissie-c
Copy link
Contributor

Closing this as it was merged in #391

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

No branches or pull requests

2 participants