@oej commented on this pull request. return dns_update_pv(naptrname, pvid); } +/** + * There is room for a description of the function here - that's why we have comments before functions. -- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/3802#pullrequestreview-1974134325 You are receiving this because you are subscribed to this thread. Message ID: <kamailio/kamailio/pull/3802/review/1974134325(a)github.com>

@oej commented on this pull request. You're doing good. Just a few small nits after reading it again. </section> + <section id="ipops.f.ptr_query"> + <title> + <function moreinfo="none">ptr_query(ip, pvid)</function> + </title> + + <para> + Store the hostname that correspond to ip s/ip/an IP address/ Also document if you support both IPv4 and IPv6 clearly. Thanks! </section> + <section id="ipops.f.ptr_query"> + <title> + <function moreinfo="none">ptr_query(ip, pvid)</function> + </title> + + <para> + Store the hostname that correspond to ip + in a config variable $ptrquery(pvid=>hostname). You mean a "pseudo variable" not a config variable. + <function moreinfo="none">ptr_query(ip, pvid)</function> + </title> + + <para> + Store the hostname that correspond to ip + in a config variable $ptrquery(pvid=>hostname). + </para> + + <para>Parameters:</para> + + <itemizedlist> + <listitem> + <para> + <emphasis>ip</emphasis> - string or pseudo-variable containing the ip. + The resulting IP addresses from DNS query are compared with ipaddr. In line 901 you just say "ip" and here "ipaddr". I suggest you write clear text on both cases. Like "containing the IP address" and "compared with the IP address" + in a config variable $ptrquery(pvid=>hostname). + </para> + + <para>Parameters:</para> + + <itemizedlist> + <listitem> + <para> + <emphasis>ip</emphasis> - string or pseudo-variable containing the ip. + The resulting IP addresses from DNS query are compared with ipaddr. + </para> + </listitem> + <listitem> + <para> + <emphasis>pvid</emphasis> - container id for script variable. I don't think we call a pseudo variable a "container" anywhere. If so, I consider it a bug :-) return 1; } +/* +* Add a comment here #define _IPOPS_PV_H_ #include "../../core/pvar.h" +#define PV_DNS_ADDR 64 +#define PV_DNS_RECS 32 +#define SR_DNS_PVIDX 1 + +typedef struct _sr_dns_record +{ + int type; + char addr[PV_DNS_ADDR]; +} sr_dns_record_t; +typedef struct _sr_dns_item Empty line missing + * + */ +static int w_ptr_query(sip_msg_t *msg, char *ip, char *pv_name) +{ + str ip_address; + str name; + + if(msg == NULL) { + LM_ERR("received null msg\n"); + return -1; + } + + if(fixup_get_svalue(msg, (gparam_t *)ip, &ip_address) < 0) { + LM_ERR("cannot get the IP address\n"); + return -1; Depends if you think there's any point of separating the various errors from each other in the script of if it doesn't matter - the coder just want to know that it failed, not caring about how. There are several functions in other modules, like TM, that has multiple return values. It's really up to you, just a suggestion that you consider this. -- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/3802#pullrequestreview-1975734865 You are receiving this because you are subscribed to this thread. Message ID: &lt;kamailio/kamailio/pull/3802/review/1975734865(a)github.com&gt;

@henningw commented on this pull request. </section> + <section id="ipops.f.ptr_query"> + <title> + <function moreinfo="none">ptr_query(ip, pvid)</function> + </title> + + <para> + Store the hostname that correspond to ip + in a config variable $ptrquery(pvid=>hostname). The term "config variable" is also already used in the module, but I agree its probably more clear to spell it explictly. -- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/3802#discussion_r1549082716 You are receiving this because you are subscribed to this thread. Message ID: &lt;kamailio/kamailio/pull/3802/review/1975789445(a)github.com&gt;

@oej commented on this pull request. </section> + <section id="ipops.f.ptr_query"> + <title> + <function moreinfo="none">ptr_query(ip, pvid)</function> + </title> + + <para> + Store the hostname that correspond to ip + in a config variable $ptrquery(pvid=>hostname). You are right. But it doesn't make it more correct... :-) xkaraman - you decide if you want to improve the docs or keep it at the current state. -- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/3802#discussion_r1549218619 You are receiving this because you are subscribed to this thread. Message ID: &lt;kamailio/kamailio/pull/3802/review/1975961572(a)github.com&gt;

@xkaraman commented on this pull request. return 1; } +/* +* Fixed -- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/3802#discussion_r1549285664 You are receiving this because you are subscribed to this thread. Message ID: &lt;kamailio/kamailio/pull/3802/review/1976068485(a)github.com&gt;

@xkaraman commented on this pull request. </section> + <section id="ipops.f.ptr_query"> + <title> + <function moreinfo="none">ptr_query(ip, pvid)</function> + </title> + + <para> + Store the hostname that correspond to ip + in a config variable $ptrquery(pvid=>hostname). Fixed -- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/3802#discussion_r1549287626 You are receiving this because you are subscribed to this thread. Message ID: &lt;kamailio/kamailio/pull/3802/review/1976071728(a)github.com&gt;