libcs are implementing changes to fix the year 2038 issue on 32 bit platforms (see [1]). musl libc already went ahead and implemented it, starting with musl-1.2.0 (see [2]).
This commit adds a new definition to src/core/dprint.h:
TIME_T_INT_FMT
If __USE_TIME_BITS64 is defined (by a time64 libc, see [1]), it is set to the proper conversion for type int64_t, PRId64. If __USE_TIME_BITS64 is not defined, the status quo remains unchanged ("%ld" is used).
Note: In some places kamailio uses "%li" instead of "%ld". But in the context of printf etc. there is no difference, so this commit replaces "%li" with "%ld".
The new definition is used in the different parts of kamailio, where appropriate.
These changes get rid of the new warnings that appeared with musl-1.2.0. Below an example warning:
In file included from auth_identity.c:50: auth_identity.c: In function 'check_date': ../../core/dprint.h:316:73: warning: format '%ld' expects argument of type 'long int', but argument 11 has type 'time_t' {aka 'long long int'} [-Wformat=] 316 | fprintf(stderr, "%2d(%d) %s: %.*s%s%s%s" fmt, \ | ^~~~~~~~~~~~~~~~~~~~~~~~ ../../core/dprint.h:340:25: note: in expansion of macro 'LOG_FX' 340 | LOG_FX(facility, level, lname, prefix, _FUNC_NAME_, fmt, ## args) | ^~~~~~ ../../core/dprint.h:346:25: note: in expansion of macro 'LOG_FL' 346 | LOG_FL(facility, level, NULL, prefix, fmt, ## args) | ^~~~~~ ../../core/dprint.h:349:25: note: in expansion of macro 'LOG_FP' 349 | LOG_FP(DEFAULT_FACILITY, (level), LOC_INFO, fmt, ## args) | ^~~~~~ auth_identity.c:594:17: note: in expansion of macro 'LOG' 594 | LOG(L_INFO, "AUTH_IDENTITY VERIFIER: Outdated date header value (%ld sec)\n", tnow - tmsg + glb_iauthval); | ^~~
[1] https://sourceware.org/glibc/wiki/Y2038ProofnessDesign [2] https://musl.libc.org/time64.html
Signed-off-by: Sebastian Kemper <sebastian_ml@gmx.net>
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail -->
Hi all,
This adds time64 libc support. Should be non-breaking, status quo remains. I hope you don't mind a made up a new patch category, "time".
I did not run-test this (pretty busy right now). But the warnings go away.
If absolutely run-testing is needed for this, let me know.
Kind regards, Seb You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2894
-- Commit Summary --
* <a href="https://github.com/kamailio/kamailio/pull/2894/commits/27175614787f6b05795f1e019e7d17df32d19be6">time: add support for time64 libc</a>
-- File Changes --
M src/core/dprint.h (8) M src/modules/auth_identity/auth_identity.c (2) M src/modules/cdp/routing.c (4) M src/modules/db_redis/redis_dbase.c (2) M src/modules/ims_charging/ims_ro.c (2) M src/modules/ims_dialog/dlg_handlers.c (2) M src/modules/ims_icscf/scscf_list.c (4) M src/modules/ims_registrar_pcscf/notify.c (2) M src/modules/ims_registrar_pcscf/save.c (2) M src/modules/ims_registrar_scscf/registrar_notify.c (3) M src/modules/ims_registrar_scscf/save.c (2) M src/modules/ims_usrloc_scscf/impurecord.c (4) M src/modules/nat_traversal/nat_traversal.c (2) M src/modules/xhttp_pi/xhttp_pi_fnc.c (4)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2894.patch https://github.com/kamailio/kamailio/pull/2894.diff
Thanks for the pull request. Just inventing a new patch category is not the right approach, sorry.
Please just use "core" for the core part and the other module parts as individual commits per module with the respective module name as prefix.
One small question, why do you add this additional quotes and space around the TIME_T_INT_FMT #define?
Just for reference, a similar patch for asterisk can be found [here](https://gerrit.asterisk.org/c/asterisk/+/16621)
Thanks for the pull request. Just inventing a new patch category is not the right approach, sorry.
Please just use "core" for the core part and the other module parts as individual commits per module with the respective module name as prefix.
No problem, will do.
One small question, why do you add this additional quotes and space around the TIME_T_INT_FMT #define?
You mean this, right?
LM_DBG("converting datetime value %" TIME_T_INT_FMT " to str\n", VAL_TIME(v));
Below wouldn't work: LM_DBG("converting datetime value %TIME_T_INT_FMT to str\n", VAL_TIME(v));
Or maybe I didn't understand your question?
Kind regards, Seb
Thanks for the pull request. Just inventing a new patch category is not the right approach, sorry.
Changed to core.
Please just use "core" for the core part and the other module parts as individual commits per module with the respective module name as prefix.
Done.
Kind regards, Seb
Somehow I find _**ugly**_ how the format string becomes. For printing, I would rather cast the value/expression to be printed to `int64_t` and use `%lld` inside formatting string. But probably it is not straight for `scan` functions. So it is more a cosmetic opinion.
From my point of view can be merged as it is and in the future we can change if we find a better alternative.
Maybe to make it shorter a bit, the `TIME_T_INT_FMT` can be renamed to `TIME_T_FMT` if does not conflict with an existing macro, I don't see a reason to have INT inside the name because it is about the format for printing a `time_t` value.
This rename can be done later, after merging this PR.
Maybe to make it shorter a bit, the `TIME_T_INT_FMT` can be renamed to `TIME_T_FMT` if does not conflict with an existing macro, I don't see a reason to have INT inside the name because it is about the format for printing a `time_t` value.
This rename can be done later, after merging this PR.
Hello Daniel,
Thanks for the feedback. I can change the definition name if you want, no problem.
Currently I'm finding out the hard way (with another software) that fixing the new warnings may not be enough, the application may still not work right on 32 bit computers. I didn't run-test kamailio yet, so it may be OK or maybe it's not. But the commit message "promise" ("add support for time64 libc") may be promising too much :) We'll see.
Kind regards, Seb
The problem I had turned out to be cruft from old builds in my buildroot :)
Anyway, meantime I got some extra feedback from musl mailing list ([1]). Basically what you also said, use "lld" and cast. So I changed the commits to do that. But I did keep the definition (although I renamed it as per your suggestion).
And then the scan issue. When I compiled for a 64 bit target I got a warning there. So I looked around and found a solution here ([2]). So I went with this in the nat_traversal module. Please verify.
I run-tested it, but honestly that may not mean much.
``` opkg list-installed|grep kamailio kamailio - 5.5.2-1 kamailio-mod-ctl - 5.5.2-1 kamailio-mod-db-sqlite - 5.5.2-1 kamailio-mod-dialplan - 5.5.2-1 kamailio-mod-jsonrpcs - 5.5.2-1 kamailio-mod-maxfwd - 5.5.2-1 kamailio-mod-path - 5.5.2-1 kamailio-mod-permissions - 5.5.2-1 kamailio-mod-pv - 5.5.2-1 kamailio-mod-rr - 5.5.2-1 kamailio-mod-rtpproxy - 5.5.2-1 kamailio-mod-sanity - 5.5.2-1 kamailio-mod-sipdump - 5.5.2-1 kamailio-mod-siputils - 5.5.2-1 kamailio-mod-sl - 5.5.2-1 kamailio-mod-textops - 5.5.2-1 kamailio-mod-textopsx - 5.5.2-1 kamailio-mod-tm - 5.5.2-1 kamailio-mod-tmx - 5.5.2-1 kamailio-mod-xlog - 5.5.2-1 ```
So I'm not using any of the modules that I patch I'm afraid :/
[1] https://www.openwall.com/lists/musl/2021/10/27/3 [2] https://stackoverflow.com/questions/4171478/how-to-read-data-into-a-time-t-v...
Thanks, merging!
Merged #2894 into master.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
micmac1