Thanks, really appreciated being a module I am not using much, so I haven't approach it from KEMI perspective yet! @lazedo - can you quickly check this PR and see if there is something you want to comment on? Otherwise, I am fine to merge. -- 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/2324#issuecomment-627299210

@lazedo requested changes on this pull request. thank you for your contribution. some required changes + { + LM_ERR("failed to init kazoo local sip msg\n"); + return -1; + } + why was this added ? + if(kazoo_event_callback.s!=NULL && kazoo_event_callback.len>0) { + keng = sr_kemi_eng_get(); + if(keng==NULL) { + LM_ERR("failed to find kemi engine\n"); + return -1; + } + kazoo_route_no=-1; + } + else { + route_no=route_lookup(&event_rt, "kazoo:consumer-event"); + if (route_no==-1) + { + LM_ERR("failed to find event_route[kazoo:consumer-event]\n"); + return -1; + } + if (event_rt.rlist[route_no]==0) + { + LM_WARN("event_route[kazoo:consumer-event] is empty\n"); + } + kazoo_route_no=route_no; + } + + please use a different logic here. `kazoo:consumer-event` is not mandatory. you could set a variable like `kz_use_kemi` that is set if `kazoo_event_callback` is set and `kemi` is valid. then use that variable when needed + rt = route_get(&event_rt, "kazoo:mod-init"); + if(rt>=0 && event_rt.rlist[rt]!=NULL) { + LM_DBG("executing event_route[kazoo:mod-init] (%d)\n", rt); formatting looks odd sprintf(buffer, "kazoo:consumer-event-%.*s-%.*s",ev_category.len, ev_category.s, ev_name.len, ev_name.s); - for (p=buffer ; *p; ++p) *p = tolower(*p); - for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; - if(kz_amqp_consumer_fire_event(buffer) != 0) { - sprintf(buffer, "kazoo:consumer-event-%.*s",ev_category.len, ev_category.s); for (p=buffer ; *p; ++p) *p = tolower(*p); for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; if(kz_amqp_consumer_fire_event(buffer) != 0) { - sprintf(buffer, "kazoo:consumer-event-%s-%s", key, subkey); + sprintf(buffer, "kazoo:consumer-event-%.*s",ev_category.len, ev_category.s); for (p=buffer ; *p; ++p) *p = tolower(*p); for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; if(kz_amqp_consumer_fire_event(buffer) != 0) { - sprintf(buffer, "kazoo:consumer-event-%s", key); + sprintf(buffer, "kazoo:consumer-event-%s-%s", key, subkey); for (p=buffer ; *p; ++p) *p = tolower(*p); for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; - if(kz_amqp_consumer_fire_event(buffer) != 0) { - sprintf(buffer, "kazoo:consumer-event"); - if(kz_amqp_consumer_fire_event(buffer) != 0) { - LM_ERR("kazoo:consumer-event not found\n"); - } - } + if(kz_amqp_consumer_fire_event(buffer) != 0) { + sprintf(buffer, "kazoo:consumer-event-%s", key); + for (p=buffer ; *p; ++p) *p = tolower(*p); + for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; + if(kz_amqp_consumer_fire_event(buffer) != 0) { + sprintf(buffer, "kazoo:consumer-event"); + if(kz_amqp_consumer_fire_event(buffer) != 0) { + LM_ERR("kazoo:consumer-event not found\n"); + } + } + } } } + } else { + keng = sr_kemi_eng_get(); + if(keng!=NULL) { + sip_msg_t *msg; + str evrtname = str_init("kazoo:consumer-event"); + + rtb = get_route_type(); + msg = faked_msg_next(); + if(sr_kemi_route(keng, msg, EVENT_ROUTE, &kazoo_event_callback, &evrtname)<0) { + LM_ERR("error running event route kemi callback\n"); + } + set_route_type(rtb); + } else { + LM_ERR("no event route or kemi callback found for execution\n"); + } } + split this in two different functions that are evoked depending on `kemi` being active - if(rt>=0 && event_rt.rlist[rt]!=NULL) { - LM_DBG("executing event_route[kazoo:mod-init] (%d)\n", rt); - if(faked_msg_init()<0) - return -1; - fmsg = faked_msg_next(); - rtb = get_route_type(); - set_route_type(REQUEST_ROUTE); - init_run_actions_ctx(&ctx); - run_top_route(event_rt.rlist[rt], fmsg, &ctx); - if(ctx.run_flags&DROP_R_F) - { - LM_ERR("exit due to 'drop' in event route\n"); - return -1; + if (kazoo_route_no>=0) { + rt = route_get(&event_rt, "kazoo:mod-init"); + if(rt>=0 && event_rt.rlist[rt]!=NULL) { + LM_DBG("executing event_route[kazoo:mod-init] (%d)\n", rt); + if(faked_msg_init()<0) + return -1; + fmsg = faked_msg_next(); + rtb = get_route_type(); + set_route_type(REQUEST_ROUTE); + init_run_actions_ctx(&ctx); + run_top_route(event_rt.rlist[rt], fmsg, &ctx); + if(ctx.run_flags&DROP_R_F) + { + LM_ERR("exit due to 'drop' in event route\n"); + return -1; + } + set_route_type(rtb); + } + } + else { + keng = sr_kemi_eng_get(); + if(keng!=NULL) { + sip_msg_t *msg; + str evrtname = str_init("kazoo:mod-init"); + rtb = get_route_type(); + msg = faked_msg_next(); + if(sr_kemi_route(keng, msg, EVENT_ROUTE, &kazoo_event_callback, &evrtname)<0) { + LM_ERR("error running event route kemi callback\n"); + } + set_route_type(rtb); + } + else { + LM_ERR("no event route or kemi callback found for execution\n"); split this in two different function that are evoked by `fire_init_event` depending on `kemi` being active -- 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/2324#pullrequestreview-414207173

I have addressed the suggested changes in the new commit: - removed route_lookup for kazoo:consumer-event as it may or may not exist - depending on using cfg or kemi: - fire_init_event() calls fire_init_event_cfg() or fire_init_event_kemi() - kz_amqp_consumer_event() calls kz_amqp_consumer_event_cfg() or and tidied up formatting. Tested on my test servers for: kazoo_subscribe, kazoo_publish, kazoo:consumer-event, with the traditional cfg and Kemi (Lua). kz_amqp_consumer_event_kemi() -- 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/2324#issuecomment-631588930

@tao-communications commented on this pull request. + if(kazoo_event_callback.s!=NULL && kazoo_event_callback.len>0) { + keng = sr_kemi_eng_get(); + if(keng==NULL) { + LM_ERR("failed to find kemi engine\n"); + return -1; + } + kazoo_route_no=-1; + } + else { + route_no=route_lookup(&event_rt, "kazoo:consumer-event"); + if (route_no==-1) + { + LM_ERR("failed to find event_route[kazoo:consumer-event]\n"); + return -1; + } + if (event_rt.rlist[route_no]==0) + { + LM_WARN("event_route[kazoo:consumer-event] is empty\n"); + } + kazoo_route_no=route_no; + } + + Now removed route_lookup for kazoo:consumer-event as it may or may not exist. -- 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/2324#discussion_r428155232

@tao-communications commented on this pull request. sprintf(buffer, "kazoo:consumer-event-%.*s-%.*s",ev_category.len, ev_category.s, ev_name.len, ev_name.s); - for (p=buffer ; *p; ++p) *p = tolower(*p); - for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; - if(kz_amqp_consumer_fire_event(buffer) != 0) { - sprintf(buffer, "kazoo:consumer-event-%.*s",ev_category.len, ev_category.s); for (p=buffer ; *p; ++p) *p = tolower(*p); for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; if(kz_amqp_consumer_fire_event(buffer) != 0) { - sprintf(buffer, "kazoo:consumer-event-%s-%s", key, subkey); + sprintf(buffer, "kazoo:consumer-event-%.*s",ev_category.len, ev_category.s); for (p=buffer ; *p; ++p) *p = tolower(*p); for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; if(kz_amqp_consumer_fire_event(buffer) != 0) { - sprintf(buffer, "kazoo:consumer-event-%s", key); + sprintf(buffer, "kazoo:consumer-event-%s-%s", key, subkey); for (p=buffer ; *p; ++p) *p = tolower(*p); for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; - if(kz_amqp_consumer_fire_event(buffer) != 0) { - sprintf(buffer, "kazoo:consumer-event"); - if(kz_amqp_consumer_fire_event(buffer) != 0) { - LM_ERR("kazoo:consumer-event not found\n"); - } - } + if(kz_amqp_consumer_fire_event(buffer) != 0) { + sprintf(buffer, "kazoo:consumer-event-%s", key); + for (p=buffer ; *p; ++p) *p = tolower(*p); + for (p=buffer ; *p; ++p) if(*p == '_') *p = '-'; + if(kz_amqp_consumer_fire_event(buffer) != 0) { + sprintf(buffer, "kazoo:consumer-event"); + if(kz_amqp_consumer_fire_event(buffer) != 0) { + LM_ERR("kazoo:consumer-event not found\n"); + } + } + } } } + } else { + keng = sr_kemi_eng_get(); + if(keng!=NULL) { + sip_msg_t *msg; + str evrtname = str_init("kazoo:consumer-event"); + + rtb = get_route_type(); + msg = faked_msg_next(); + if(sr_kemi_route(keng, msg, EVENT_ROUTE, &kazoo_event_callback, &evrtname)<0) { + LM_ERR("error running event route kemi callback\n"); + } + set_route_type(rtb); + } else { + LM_ERR("no event route or kemi callback found for execution\n"); + } } + Now depending on using cfg or kemi: - kz_amqp_consumer_event() calls kz_amqp_consumer_event_cfg() or kz_amqp_consumer_event_kemi() -- 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/2324#discussion_r428156464

@tao-communications great job, thank you. i do have a final request, when that's done, please squash your commits into a single commit, `kazoo: add basic kemi support` i feel that `kazoo_route_no` doesn't express well the intent for the variable which is to use `kemi` instead of `cfg`. can you please rename `kazoo_route_no` to `kazoo_kemi_enabled` with default 0 and set it to 1 in [here](https://github.com/kamailio/kamailio/pull/2324/files#diff-52d37ad2407… and then use ``` if ( kazoo_kemi_enabled ) { do kemi routine } else { do cfg routine } ``` where applicable, for example in `fire_init_event` ``` if (kazoo_kemi_enabled) { return fire_init_event_kemi(); } else { return fire_init_event_cfg(); } `` -- 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/2324#issuecomment-631961748

Thanks for reviewing! I made a new commit to address these. Please let me know if there are further things that need doing. -- 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/2324#issuecomment-632010822

@lazedo: @tao-communications does not have commit access yet. You can squash all commits via github merge button, just click on arrow down next to `Merge pull request` and choose `Squash and merge`. It is no need to have the squash done elsewhere. I can do it if you prefer. -- 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/2324#issuecomment-632028215

@miconda why did you dismissed my review ? -- 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/2324#issuecomment-632040899

@lazedo - I dismissed the review because it was requesting an external squash, which is not necessary in my opinion. You can set the any commit message when you squash and merge via github.com web UI. -- 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/2324#issuecomment-632042767

I have attempted a git squash and hopefully it's OK. -- 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/2324#issuecomment-632115862

Merged #2324 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/2324#event-3360912478

Thank you very much @lazedo and @miconda for the review and support! -- 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/2324#issuecomment-632125456