@oej commented on this pull request.

@@ -1292,6 +1297,31 @@ static int ki_dns_query(sip_msg_t *msg, str *naptrname, str *pvid)

return dns_update_pv(naptrname, pvid); }

+/** + *

There is room for a description of the function here - that's why we have comments before functions.

@oej commented on this pull request.

You're doing good. Just a few small nits after reading it again.

@@ -883,6 +883,57 @@ if(dns_query("test.com", "xyz"))

</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!

@@ -883,6 +883,57 @@ if(dns_query("test.com", "xyz"))

</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.

•  <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). + </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"

•   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. + </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 :-)

@@ -418,6 +413,91 @@ int dns_update_pv(str *hostname, str *name)

return 1; }

+/* +*

Add a comment here

@@ -24,13 +24,46 @@

#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.

@henningw commented on this pull request.

@@ -883,6 +883,57 @@ if(dns_query("test.com", "xyz"))

</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.

@oej commented on this pull request.

@@ -883,6 +883,57 @@ if(dns_query("test.com", "xyz"))

</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.

@xkaraman commented on this pull request.

@@ -418,6 +413,91 @@ int dns_update_pv(str *hostname, str *name)

return 1; }

+/* +*

Fixed

@xkaraman commented on this pull request.

@@ -883,6 +883,57 @@ if(dns_query("test.com", "xyz"))

</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