[Asterisk-code-review] res xmpp: Correct implementation of JABBER STATUS & JabberSt... (asterisk[master])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5296 )

Change subject: res_xmpp: Correct implementation of JABBER_STATUS & JabberStatus
......................................................................


res_xmpp: Correct implementation of JABBER_STATUS & JabberStatus

The documentation for JABBER_STATUS (and the deprecated JabberStatus
app) indicate that a return value of 7 indicates that the specified
buddy was not in the roster. It also indicates that you can specify a
"bare" JID (one without a resource). Unfortunately the actual behavior
does not match the documented behavior.

Assuming that our roster includes the buddy online and available
"valid at example.org/Valid" and does *not* include the buddy
"invalid at example.org", the JABBER_STATUS() function returns the
following before this patch:

+------------------------------+------------+--------------------------+
| Buddy                        | Status     | Result                   |
+------------------------------+------------+--------------------------+
| valid at example.org            |  Online    |  7 (Not in roster)       |
| valid at example.org/Valid      |  Online    |  1 (Online)              |
| valid at example.org/Invalid    |  N/A       |  7 (Not in roster)       |
| invalid at example.org          |  N/A       |  Error logged, no return |
| invalid at example.org/Valid    |  N/A       |  Error logged, no return |
+------------------------------+------------+--------------------------+

And after this patch:

+------------------------------+------------+--------------------------+
| Buddy                        | Status     | Result                   |
+------------------------------+------------+--------------------------+
| valid at example.org            |  Online    |  1 (Online)              |
| valid at example.org/Valid      |  Online    |  1 (Online)              |
| valid at example.org/Invalid    |  N/A       |  6 (Offline)             |
| invalid at example.org          |  N/A       |  7 (Not in roster)       |
| invalid at example.org/Valid    |  N/A       |  7 (Not in roster)       |
+------------------------------+------------+--------------------------+

This brings the behavior in line with the documentation.

ASTERISK-23510 #close
Reported by: Anthony Critelli

Change-Id: I9c3241035363ef4a6bdc21fabfd8ffcd9ec657bf
---
M res/res_xmpp.c
1 file changed, 31 insertions(+), 44 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/res/res_xmpp.c b/res/res_xmpp.c
index 1aa865c..3c80d4d 100644
--- a/res/res_xmpp.c
+++ b/res/res_xmpp.c
@@ -1630,6 +1630,35 @@
 	return CMP_MATCH | CMP_STOP;
 }
 
+#define BUDDY_OFFLINE 6
+#define BUDDY_NOT_IN_ROSTER 7
+
+static int get_buddy_status(struct ast_xmpp_client_config *clientcfg, char *screenname, char *resource)
+{
+	int status = BUDDY_OFFLINE;
+	struct ast_xmpp_resource *res;
+	struct ast_xmpp_buddy *buddy = ao2_find(clientcfg->client->buddies, screenname, OBJ_KEY);
+
+	if (!buddy) {
+		return BUDDY_NOT_IN_ROSTER;
+	}
+
+	res = ao2_callback(
+		buddy->resources,
+		0,
+		ast_strlen_zero(resource) ? xmpp_resource_immediate : xmpp_resource_cmp,
+		resource);
+
+	if (res) {
+		status = res->status;
+	}
+
+	ao2_cleanup(res);
+	ao2_cleanup(buddy);
+
+	return status;
+}
+
 /*
  * \internal
  * \brief Dial plan function status(). puts the status of watched user
@@ -1643,10 +1672,7 @@
 {
 	RAII_VAR(struct xmpp_config *, cfg, ao2_global_obj_ref(globals), ao2_cleanup);
 	RAII_VAR(struct ast_xmpp_client_config *, clientcfg, NULL, ao2_cleanup);
-	struct ast_xmpp_buddy *buddy;
-	struct ast_xmpp_resource *resource;
 	char *s = NULL, status[2];
-	int stat = 7;
 	static int deprecation_warning = 0;
 	AST_DECLARE_APP_ARGS(args,
 			     AST_APP_ARG(sender);
@@ -1685,25 +1711,7 @@
 		return -1;
 	}
 
-	if (!(buddy = ao2_find(clientcfg->client->buddies, jid.screenname, OBJ_KEY))) {
-		ast_log(LOG_WARNING, "Could not find buddy in list: '%s'\n", jid.screenname);
-		return -1;
-	}
-
-	if (ast_strlen_zero(jid.resource) || !(resource = ao2_callback(buddy->resources, 0, xmpp_resource_cmp, jid.resource))) {
-		resource = ao2_callback(buddy->resources, OBJ_NODATA, xmpp_resource_immediate, NULL);
-	}
-
-	ao2_ref(buddy, -1);
-
-	if (resource) {
-		stat = resource->status;
-		ao2_ref(resource, -1);
-	} else {
-		ast_log(LOG_NOTICE, "Resource '%s' of buddy '%s' was not found\n", jid.resource, jid.screenname);
-	}
-
-	snprintf(status, sizeof(status), "%d", stat);
+	snprintf(status, sizeof(status), "%d", get_buddy_status(clientcfg, jid.screenname, jid.resource));
 	pbx_builtin_setvar_helper(chan, args.variable, status);
 
 	return 0;
@@ -1722,9 +1730,6 @@
 {
 	RAII_VAR(struct xmpp_config *, cfg, ao2_global_obj_ref(globals), ao2_cleanup);
 	RAII_VAR(struct ast_xmpp_client_config *, clientcfg, NULL, ao2_cleanup);
-	struct ast_xmpp_buddy *buddy;
-	struct ast_xmpp_resource *resource;
-	int stat = 7;
 	AST_DECLARE_APP_ARGS(args,
 			     AST_APP_ARG(sender);
 			     AST_APP_ARG(jid);
@@ -1756,25 +1761,7 @@
 		return -1;
 	}
 
-	if (!(buddy = ao2_find(clientcfg->client->buddies, jid.screenname, OBJ_KEY))) {
-		ast_log(LOG_WARNING, "Could not find buddy in list: '%s'\n", jid.screenname);
-		return -1;
-	}
-
-	if (ast_strlen_zero(jid.resource) || !(resource = ao2_callback(buddy->resources, 0, xmpp_resource_cmp, jid.resource))) {
-		resource = ao2_callback(buddy->resources, OBJ_NODATA, xmpp_resource_immediate, NULL);
-	}
-
-	ao2_ref(buddy, -1);
-
-	if (resource) {
-		stat = resource->status;
-		ao2_ref(resource, -1);
-	} else {
-		ast_log(LOG_NOTICE, "Resource %s of buddy %s was not found.\n", jid.resource, jid.screenname);
-	}
-
-	snprintf(buf, buflen, "%d", stat);
+	snprintf(buf, buflen, "%d", get_buddy_status(clientcfg, jid.screenname, jid.resource));
 
 	return 0;
 }

-- 
To view, visit https://gerrit.asterisk.org/5296
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I9c3241035363ef4a6bdc21fabfd8ffcd9ec657bf
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>