[Asterisk-code-review] res_pjsip_pidf: Cisco phone presence enablement (asterisk[master])

[snuffy]

snuffy has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13162 )

Change subject: res_pjsip_pidf: Cisco phone presence enablement
......................................................................


Patch Set 5:

(2 comments)

> Patch Set 5: Code-Review-1
> 
> I think this will work but i'd like to propose an alternative...
> Instead of creating a body supplement module, create a body generator module that registers as "cpim-pidf+xml".  In that module's allocate_body callback, call ast_sip_pubsub_generate_body_content for "pidf+xml".  That will give you an ast_str with the normal pidf+xml in it.  You can then call pj_xml_parse to turn it into an XML node and return that as the body.  When your generate_body_content function gets called, you just add your cisco specific stuff.
> 
> This way you don't have to mess with res_pjsip_pubsub and mung up the provided accept based on the UA.
> 
> Regardless of the solution, the big question is... Will this apply to ALL Cisco phones and/or all phones that ask for "cpim-pidf+xml" or is it only some Cisco phones or some firmwares?


Will look into creating something like you describe but it may take some time (primarily code was PoC for others to test).

As for the second part, ALL Cisco phones.. no idea (only have access to 7941/2 and 7961/2), we would need someone who has the newer 8xxx series etc. The 'bug' appears to be across all 79xx firmware I have tried (8.x/9.x) though and the phone is rather old now.

https://gerrit.asterisk.org/c/asterisk/+/13162/3/res/res_pjsip_pubsub.c 
File res/res_pjsip_pubsub.c:

https://gerrit.asterisk.org/c/asterisk/+/13162/3/res/res_pjsip_pubsub.c@854 
PS3, Line 854: 		alloc_size = pj_strlen(&user_agent_header->hvalue) + 1;
             : 		user_agent = ast_alloca(alloc_size);
             : 		ast_copy_pj_str(user_agent, &user_agent_header->hvalue, alloc_size);
             : 
             : 		if (strstr(user_agent,"Cisco"))
> I think you could simplify this with: […]
Have updated in v5 with that style.. seems to still work.


https://gerrit.asterisk.org/c/asterisk/+/13162/3/res/res_pjsip_pubsub.c@858 
PS3, Line 858: 		if (strstr(user_agent,"Cisco"))
I realise this is probably 'too' generic but need others especially with non 79xx handsets to trial see if they complain. We could restrict to Cisco-CP79 instead if desired.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13162
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ib5a2944de0e280d3c92eed42d91a0bcb0bcf387c
Gerrit-Change-Number: 13162
Gerrit-PatchSet: 5
Gerrit-Owner: snuffy <snuffy22 at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-CC: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Wed, 13 Nov 2019 05:05:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean.bright at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191112/7af924e9/attachment.html>