on missed / failed calls issue #1673
#### 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) - [x] Breaking change (not tested with all scenarios, review could confirm that this is not introducing regressions)
#### 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 - [x] Related to issue #1673
#### Description problem with confusion when using flags (integer value vs bit position value) proposed solution is using the flags helpers functions `isflagset`, etc. where possible. You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1674
-- Commit Summary --
* acc: generating duplicates
-- File Changes --
M src/modules/acc/acc.c (8) M src/modules/acc/acc_logic.c (5) M src/modules/acc_diameter/acc_diameter_mod.c (4) M src/modules/acc_json/acc_json_mod.c (4) M src/modules/acc_radius/acc_radius_mod.c (4)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1674.patch https://github.com/kamailio/kamailio/pull/1674.diff
jchavanton commented on this pull request.
if (is_log_mc_on(req)) {
env_set_text( ACC_MISSED, ACC_MISSED_LEN); acc_log_request( req ); - flags_to_reset |= log_missed_flag; + flags_to_reset |= 1 << log_missed_flag;
I wonder why this was not creating more problem in the past the flags variable where containing "bit position", this is one point that the reviewer can help to confirm. (I may triple check that this is always true)
Hello Julien, thank you for this request.
So did I understood it correctly, you want to unify the flag manipulation and also fix some bugs this way? It is probably better to not combine this two changes. If you change different modules, these changes should be also in individual commits, if they are not interdependent on each other.
I had a quick look to the patch.
- if((type==1) && (msg->flags&(e->missed_flag))) { + if((type==1) && isflagset(msg, e->missed_flag) == 1) {
This should be identically, according to the definition of isflagset. I don't see any big advantage of one format to the other, or I am wrong?
One question about this change (it is represented several times):
- _acc_diameter_engine.missed_flag = 1<<diameter_missed_flag; + _acc_diameter_engine.missed_flag = diameter_missed_flag;
This will not produce identical results, I think. The first one is a left shift of diameter_missed_flag length, and the second one an assignment. Example:
diameter_missed_flag = 00000000000000000000000000001000 (8) _acc_diameter_engine.missed_flag = 00000000000000000000000010000000 (256)
What is your reasoning behind this change?
Hi Hennig, thanks for the review so far.
Q: "you want to unify the flag manipulation and also fix some bugs this way?" A: Correct, to fix the bug we need to change inconsitant flag manipulation creating the bug
Q: This should be identically, according to the definition of isflagset. I don't see any big advantage of one format to the other, or I am wrong? A: They are not identical :
The first one is expecting an input we the integer contain the bit mask `if((type==1) && (msg->flags&(e->missed_flag))) {` will with with the input of : 1,2,4,8,...,4294967296
The second one is expecting an input where integer contain the bit position `if((type==1) && isflagset(msg, e->missed_flag) == 1) {` will do the bit shifting and will work with the input : 1,2,3,..,32
``` int isflagset( struct sip_msg* msg, flag_t flag ) { return (msg->flags & (1<<flag)) ? 1 : -1; // << notice the bit shifting } ```
`resetflag` and `setflag` are also doing the bit shifthing, seems like at one point someone got confused and introduce a mix which endedup doing resetflag on an alredy bit shifted input. I guess this is also why acc_mod is not doing bit shifting on the module param while acc_diameter, acc_raduis and acc_json are (I am changing this in the patch)
Hello Julien, you are right - this is indeed confusing. So you are also saying (regarding my second remark) that the other acc modules are doing it wrong?
At this point it is a bit challenging to conclude what is wrong :)
The other modules, `acc_diameter`, `acc_raduis` and `acc_json` are using the `acc_engine` interface and the logic in the acc module is slightly different because it is using functions like `acc_run_engines` In these modules it was assumed that `acc_flag` and `missed_flag` should be stored bit shifted, because of the fact `is_eng_acc_on` was expecting already bit shifted values.
However the main acc module is not bit shifting them, so this is one thing not making sense.
(Problem A*) Back to normal ACC module, consider the following combination found in `acc_logic.c` : `flags_to_reset |= log_missed_flag;` + `reset_acc_flag(req, flags_to_reset);` `log_missed_flag` is what ? (depending on the module it can be 1 or 2) 1: bit shifted, then `reset_acc_flag` is not compatible `#define reset_acc_flag(_rq,_flag) (resetflag((_rq), (_flag)))` 2: no bit shifted, then we need to use the assignment operator and we can not keep track of multiple flags. In fact thanks to the review, I just realized this is not good enough for fixing normal ACC module behavior, we need to keep track of multiple flags to reset until the very end of `on_missed` in case the same flag is used for `db_missed_flag` and `log_missed_flag`, I will revisit my patch.
When I look at `acc_mod.c` and when I debug the running program. `log_missed_flag` and `db_missed_flag` are not storing bit shifted values, this is why I think we need to align the acc_engine and modules using it with the default behavior.
The main acc module logic will also be fixed at the same time since the code should not always be working well as describer in (A*)
I hope I am not getting confused :) I will definitely revisit the patch knowing it is not good enough.
@jchavanton pushed 1 commit.
0f5792c acc: fix missed calls reset flags
Just pushed a complete working solution,
tested with `acc` ``` modparam("acc", "log_missed_flag", 9) modparam("acc", "db_missed_flag", 10) ``` and with `acc_json` ``` modparam("acc_json", "acc_flag", 2) modparam("acc_json", "acc_missed_flag", 7) ``` `acc_radius` and `acc_diameter` using the exact same integration
miconda commented on this pull request.
@@ -464,7 +463,15 @@ static inline void acc_onreply_in(struct cell *t, struct sip_msg *req,
} }
- +static void reset_acc_flags(struct sip_msg *req, int flags_to_reset) { + int x; + for (x=0;x<32;x++) { + if (flags_to_reset & 1<<x) { + resetflag(req, x); + LM_DBG("reset[%d]\n", x); + } + } +}
Instead of `reset_acc_flags()` which does a loop 0..31, I would rather add a helper core function to reset the msg flags with a bitmask (in addition of reseting only one), because the operation should be just:
``` msg->flags &= ~flags_to_reset; ```
@jchavanton pushed 1 commit.
9a30079 core: adding helper function resetflags
jchavanton commented on this pull request.
@@ -464,7 +463,15 @@ static inline void acc_onreply_in(struct cell *t, struct sip_msg *req,
} }
- +static void reset_acc_flags(struct sip_msg *req, int flags_to_reset) { + int x; + for (x=0;x<32;x++) { + if (flags_to_reset & 1<<x) { + resetflag(req, x); + LM_DBG("reset[%d]\n", x); + } + } +}
indeed ! thanks
I believe this fix is ready to be merged
Thank you Julien, I will merge this later today. I will need to do it manually, in order to split the commits in modules and core.
I see, should have known ... thanks
Closed #1674.
Merged manually into git master
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Julien Chavanton