17 May
2022
17 May
'22
1:13 p.m.
<!-- 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
- [ ] Small bug fix (non-breaking change which fixes an issue)
- [X] 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 -->
- [X] 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 -->
We've come into the rare issue when a kamailio process used the old $var() value
for the current request message and messed some things up.
LOG_WARN when $var() is used uninitialized in config. I can also just LOG_NOTICE this.
Can this be brought to 5.6?
Thank you,
Stefan
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3114
-- Commit Summary --
* pv: log uninit ()
-- File Changes --
M src/modules/pv/pv_core.c (9)
M src/modules/pv/pv_svar.h (1)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3114.patch
https://github.com/kamailio/kamailio/pull/3114.diff
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/3114(a)github.com&gt;
17 May
17 May
1:26 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
This does not sound right, by design $var() is initialised to 0 and no reset on every
message. It has to be set/reset explicitely in config.
If you need something specific per sip request, the xavp/avp/xavi/xavu should be used.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128694900
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128694900(a)github.com>
1:50 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
Hi,
Indeed, from what I've checked so far, the $var() value is initialized to 0. This is
done when kamailio starts.
However, the issue we faced is more related to this excerpt from kamailio cookbooks,
pseudovariables:
```
Note: A script variable persists over the Kamailio process in which it was initialized, so
be sure of giving it a new value before reading it or you'll get the value asigned in
any other previous message processed by the same Kamailio process (pid).
```
Let me know if now its more clear what the PR tries to warn.
Thank you,
Stefan
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128719358
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128719358(a)github.com>
2:14 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
Sorry, I did not understand it as well. This is exactly as $var is supposed to work, it
contains the value from previous processing. If no value has been given to it before it
returns 0, as commented from Daniel.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128740005
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128740005(a)github.com>
2:49 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
@smititelu - that note from the docs sounds ok for me (maybe because I wrote it, although
I am not sure I did). Practically wants to say that assign explicitly a value before using
it, not to get the value set a while ago in another SIP message processing context.
If you do:
```
request_route {
...
while ($var(i) < 100) {
...
$var(i) = $var(i) + 1;
}
...
}
```
The while loop gets inside once because $var(i) is 0 first time, but then remains set to
100 for all next requests. Therefore set it to the value you want before using it:
```
request_route {
...
$var(i) = 0;
while ($var(i) < 100) {
...
$var(i) = $var(i) + 1;
}
...
}
```
Feel free to change the statement in the docs, if you consider more clarifications there,
but I don't see any benefit in storing an extra int field in the internal var
structure.
Maybe $vn() offers better the kind of variable you need. It is like $var() but can have
$null value (state).
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128770857
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128770857(a)github.com>
3:28 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
Consider this scenario:
1'st part:
```
if (is_present_hf("SOME_HEADER")) {
$var(data) = _get_data_from_some_header_;
}
```
2'nd part, later:
```
if ($var(data) != "") {
_do_something_with_data_
}
```
2'nd part gets called even for SIP that does not have SOME_HEADER header, with value
set in previous SIP that had SOME_HEADER.
I wanted to log a warning for this, so if SIP with no SOME_HEADER header comes in first,
in one routing process, I will see the warning log and know to set
```$var(data)="";``` before the 1'st part.
Thank you,
Stefan
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128807385
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128807385(a)github.com>
4:06 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
Hi @miconda . Is it because of memory consumption that you don't want to store an
additional _int_ inside the _var_ structure?
The reason we came up with this PR is a strange incident we had that was rather difficult
to debug because we, you could say, wrongly used script variables.
Reading the docs is important, sure. But I think the project would benefit from this
feature because it could help users to debug or avoid unwanted strange scenarios.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128845517
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128845517(a)github.com>
4:16 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
This $var variables are supposed to hold the value from the previous processing. So its a
valid usecase if the variable was not inialized in this particular process. Would this not
cause many false alarms then?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128857043
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128857043(a)github.com>
4:32 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
It is not an alarm, it is just a log message warning the user that he uses something that
is not initialized. I once used a while(true) statement in the configuration file, which
also was a valid statement and intended by me. Back then I saw that there was a warning
printed when starting Kamailio. That's where I saw that Kamailio does some checking
this and Perls _use strict_ statement kind of motivated us to implement this feature.
Would it be OK if we would make it also configurable?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128875266
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128875266(a)github.com>
4:58 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
@smititelu, @pkuzak - it is a misunderstanding of what $var() is supposed to do and how it
acts. It is not only about an extra int field, it is about the concept behind $var().
What @smititelu gave as an example is what the note in docs try to say and what I gave as
an example above.
It can create big problems no matter is with the warning or not in the C code, because it
still keeps the value from previous assignment.
Let's say you have this situation:
- first request has SOME_HEADER (the variable is assigned, and by that no warning log)
- the next request is without SOME_HEADER, but the var still has the value from first
request and practically, during processing the 2nd request you use wrong value
Coming back to @smititelu example, you have to set var to a value that you can compare
with, like:
```
$var(data)="";
if (is_present_hf("SOME_HEADER")) {
$var(data) = _get_data_from_some_header_;
}
if ($var(data) != "") {
_do_something_with_data_
}
```
Which is same concept as with my `while` example above.
Or use $x/avp() without setting a value and compare with $null.
As I said, I don't see any benefit with the PR, irrelevant of being optional or not.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128906490
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128906490(a)github.com>
5:11 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
@miconda indeed you are right, but it will help in this situation:
- first request has not SOME_HEADER
- try to use a $var which value has not been explicitly set before => log a warning
If we start kamailio with e.g. 64 (routing) child processes there are 64 first SIP
messages that might or might not have SOME_HEADER => some will print warning, some will
not => if really unlucky, all will not :D
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128922137
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128922137(a)github.com>
5:24 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
@smititelu: I don't think that such lucky/unlucky-based logic is even supposed to be
considered for the c code.
If you have some traffic, let's say 10 requests/second, if first one is without the
header, you get the warning, the second is with the header and no more warnings, that
means in 1 second, 8 requests were processed with wrong value.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128937204
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128937204(a)github.com>
5:31 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
Yes, I agree. The patch is just meant to merely give you a config hint, at best effort.
Won't solve the wrong value problem or change $var() behavior in any way.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128945491
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128945491(a)github.com>
5:38 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
From $var() design point of view, as @henningw said, it
is bringing more confusions, because $var() is always initialised.
But you can also add a new variable that you think it meets better your needs.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128954853
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128954853(a)github.com>
5:49 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
Ok. I am closing the PR for now. Thanks a lot for the feedback.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#issuecomment-1128967037
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/c1128967037(a)github.com>
5:49 p.m.
New subject: [kamailio/kamailio] pv: log uninit $var() (PR #3114)
Closed #3114.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3114#event-6625214860
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3114/issue_event/6625214860(a)github.com>
926
days inactive
926
days old
15 comments
4 participants
participants (4)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Pawel Kuzak
-
Stefan Mititelu