<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
Hi Peter,<br>
<br>
<br>
Nice work with this patch. I have looked through it and I have some
observations.<br>
<br>
First of all, an implementation of this RFC already existed even in
Kamailio :) . I knew I coded it, but was not sure if before or after
the split, but I looked now and it is present in kamailio also. In
the README check after pidf_manipulation:<br>
<br>
<a class="moz-txt-link-freetext" href="http://git.sip-router.org/cgi-bin/gitweb.cgi?p=sip-router;a=blob;f=modules_k/presence_xml/README;h=84b0da2603e1729d2e374c9ce4f7418bf43b1922;hb=refs/heads/master">http://git.sip-router.org/cgi-bin/gitweb.cgi?p=sip-router;a=blob;f=modules_k/presence_xml/README;h=84b0da2603e1729d2e374c9ce4f7418bf43b1922;hb=refs/heads/master</a><br>
<br>
The implementation that I've done is very different from yours. In
some aspects I like your solution better. Analyzing both, I thought
at a way of combining them and making the best solution for this. In
any case, I don't think it is ok to keep them both in this state as
it will be a bit confusing for someone reading the documentation.<br>
<br>
The good think about my solution was that I left the XCAP document
fetch part in <b>presence_xml</b> module. I think it is better like
this because the <b>presence</b> module remains independent of the
XCAP part. And more than this, it works for both integrated and not
integrated XCAP server ( because it uses the generic API of fetching
XCAP documents) and even for any event (not only presence). <br>
<br>
The code for fetching the file is at line <b>473</b>:<br>
<a class="moz-txt-link-freetext" href="http://git.sip-router.org/cgi-bin/gitweb.cgi?p=sip-router;a=blob;f=modules_k/presence_xml/notify_body.c;h=fd69ed02681eb89c83071a7223b2d2df1aed6e99;hb=refs/heads/master">http://git.sip-router.org/cgi-bin/gitweb.cgi?p=sip-router;a=blob;f=modules_k/presence_xml/notify_body.c;h=fd69ed02681eb89c83071a7223b2d2df1aed6e99;hb=refs/heads/master</a><br>
<br>
The bad thing about my implementation was that I put the fetching of
this file when aggregating the presence documents. So a query in
xcap table ( for integrated xcap server) was actually performed each
time a Notify was sent out.<br>
<br>
Here I like better that you have fetched the document and stored it
in presentity table with expires -1.<br>
<br>
As for the fact that you added a new function to be called from the
script when to fetch and store this document
(pres_update_presentity), I think it would have been better to use
the existing <b>pres_refresh_watchers</b> function. If you look in
the documentation, actually when type!= 0 , a pidf manipulation
document change might be expected:<br>
<br>
<a class="moz-txt-link-freetext" href="http://git.sip-router.org/cgi-bin/gitweb.cgi?p=sip-router;a=blob;f=modules_k/presence/README;h=25de125997927321c63938e5a013f93c10b86379;hb=refs/heads/master">http://git.sip-router.org/cgi-bin/gitweb.cgi?p=sip-router;a=blob;f=modules_k/presence/README;h=25de125997927321c63938e5a013f93c10b86379;hb=refs/heads/master</a><br>
<br>
The reason why it is better to use this function is that it is
actually called by the refereshWatchers MI command and we will
maintain the compatibility with other external XCAP servers that use
this MI command. We can also put a specific type, let's say type=2
for pidf manipulation document change and type=1 for presentity
table change.<br>
<br>
Considering this the solution that I see best is the following:<br>
<br>
When <b>pres_refresh_watchers</b> is called with<b> </b>type=<b>2</b>
, try to fetch the pidf manipulation document from xcap server and
if found, store it in presentity table with expires=-1 (as you have
done). The only thing that I would change is to use a wrapper over <b>get_rules_doc</b>
function from presence_xml module. We can do this adding a new field
in the pres_ev_t structure (exactly as get_rules_doc field).<br>
<br>
Then continue with the <span class="Apple-style-span" style="color:
rgb(0, 0, 0); font-family: monospace; font-size: 12px; font-style:
normal; font-variant: normal; font-weight: normal; letter-spacing:
normal; line-height: normal; orphans: 2; text-indent: 0px;
text-transform: none; white-space: pre; widows: 2; word-spacing:
0px; background-color: rgb(255, 255, 255); display: inline !
important; float: none;">query_db_notify(pres, ev, NULL)</span>
function which will also be called if type=1.<br>
<br>
And to maintain compatibility for refereshWatchers function we can
call from here pres_referesh_watchers with type=2, considering that
also a pidf manipulation document <br>
<br>
The only thing that I don't know is how to keep from your
implementation is the possibility for a user to have multiple pidf
documents. I saw that you use the URL to fetch from xcap table and
the file name as ETAG to offer the possibility of storing multiple
pidf documents. However my question is actually whether this use
case is actually possible? Why would a user have more permanent
state documents at the same time?<br>
<br>
Let me know your thoughts about this.<br>
<br>
Regards,<br>
Anca<br>
<br>
On 04/10/2012 08:09 PM, Peter Dunkley wrote:
<blockquote cite="mid:20120410170951.59FF4EF804E@rimmer.ryngle.com"
type="cite">
<pre wrap="">Module: sip-router
Branch: master
Commit: 37812cef5fb1ee2022592de24bbae48352e17524
URL: <a class="moz-txt-link-freetext" href="http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=37812cef5fb1ee2022592de24bbae48352e17524">http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=37812cef5fb1ee2022592de24bbae48352e17524</a>
Author: Peter Dunkley <a class="moz-txt-link-rfc2396E" href="mailto:peter.dunkley@crocodile-rcs.com"><peter.dunkley@crocodile-rcs.com></a>
Committer: Peter Dunkley <a class="moz-txt-link-rfc2396E" href="mailto:peter.dunkley@crocodile-rcs.com"><peter.dunkley@crocodile-rcs.com></a>
Date: Tue Apr 10 18:05:10 2012 +0100
modules_k/presence: RFC 4827 (presence hard-state) support
- Hard-state presence documents are stored as pidf-manipulation
documents in the integrated XCAP server.
- When one of these documents is put/deleted/changed it can
be "published" using the new pres_update_presentity() exported
function.
- Because the original document is in XCAP a client can download
it and manipulate it directly.
- Hard-state documents have an expiry time of -1 and never expire
(the clean function in presence has been updated to make sure of
this).
- The filename of the document is used as the ETag value in the
presentity table. This enables multiple hard-state documents
(with different filenames) to be uploaded for each subscriber.
- Hard-state is useful for permanently setting an avatar, or an
out-of-office message, etc.
---
modules_k/presence/README | 302 ++++++++++++++++++-----------
modules_k/presence/doc/presence_admin.xml | 99 ++++++++++
modules_k/presence/presence.c | 128 ++++++++++++-
modules_k/presence/presence.h | 5 +
modules_k/presence/presentity.c | 87 ++++++---
modules_k/presence/publish.c | 200 +++++++++++++++++++-
modules_k/presence/publish.h | 2 +-
7 files changed, 679 insertions(+), 144 deletions(-)
Diff: <a class="moz-txt-link-freetext" href="http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commitdiff;h=37812cef5fb1ee2022592de24bbae48352e17524">http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commitdiff;h=37812cef5fb1ee2022592de24bbae48352e17524</a>
_______________________________________________
sr-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:sr-dev@lists.sip-router.org">sr-dev@lists.sip-router.org</a>
<a class="moz-txt-link-freetext" href="http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev">http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev</a>
</pre>
</blockquote>
<br>
</body>
</html>