22 Oct
2021
22 Oct
'21
8:12 p.m.
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(a)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/271756147…quot;>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
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894
22 Oct
22 Oct
9:13 p.m.
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)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#issuecomment-949898276
9:43 p.m.
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
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#issuecomment-949915253
23 Oct
23 Oct
1:54 a.m.
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
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#issuecomment-950018403
27 Oct
27 Oct
2:44 p.m.
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#issuecomment-952888105
2:47 p.m.
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#issuecomment-952890546
11:50 p.m.
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
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#issuecomment-953337483
31 Oct
31 Oct
10:16 p.m.
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-…
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#issuecomment-955792884
3 Nov
3 Nov
1:03 p.m.
Thanks, merging!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#issuecomment-958965440
1:03 p.m.
Merged #2894 into master.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2894#event-5561410434
930
days inactive
942
days old
9 comments
3 participants
participants (3)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
micmac1