jchavanton commented on this pull request. 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) -- 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/1674#pullrequestreview-164008683

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) -- 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/1674#issuecomment-429329716

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. -- 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/1674#issuecomment-429399926

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 -- 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/1674#issuecomment-429423775

@jchavanton pushed 1 commit. 9a30079 core: adding helper function resetflags -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/kamailio/kamailio/pull/1674/files/d278467883a45db4088f46…

I believe this fix is ready to be merged -- 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/1674#issuecomment-434961809

I see, should have known ... thanks -- 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/1674#issuecomment-435115155

Merged manually into git 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/1674#issuecomment-435176001