@oej commented on this pull request.

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

In src/modules/ipops/doc/ipops_admin.xml:

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

In src/modules/ipops/doc/ipops_admin.xml:

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

In src/modules/ipops/doc/ipops_admin.xml:

> +      <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"

In src/modules/ipops/doc/ipops_admin.xml:

> +		  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 :-)

In src/modules/ipops/ipops_pv.c:

> @@ -418,6 +413,91 @@ int dns_update_pv(str *hostname, str *name)
 	return 1;
 }
 
+/*
+*

Add a comment here

In src/modules/ipops/ipops_pv.h:

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

In src/modules/ipops/ipops_mod.c:

> +/**
+ *
+ */
+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, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.