[sr-dev] git:master: modules_k/presence: RFC 4827 (presence hard-state) support

Anca Vamanu anca.vamanu at 1and1.ro
Wed Apr 11 15:55:00 CEST 2012


Hi Peter,


Nice work with this patch. I have looked through it and I have some 
observations.

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:

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

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.

The good think about my solution was that I left the XCAP document fetch 
part in *presence_xml* module. I think it is better like this because 
the *presence* 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).

The code for fetching the file  is at line *473*:
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

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.

Here I like better that you have fetched the document and stored it in 
presentity table with expires -1.

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 
*pres_refresh_watchers* function. If you look in the documentation, 
actually when type!= 0 , a pidf manipulation document change might be 
expected:

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

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.

Considering this the solution that I see best is the following:

When *pres_refresh_watchers* is called with**type=*2* , 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 *get_rules_doc* 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).

Then continue with the query_db_notify(pres, ev, NULL) function which 
will also be called if type=1.

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

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?

Let me know your thoughts about this.

Regards,
Anca

On 04/10/2012 08:09 PM, Peter Dunkley wrote:
> Module: sip-router
> Branch: master
> Commit: 37812cef5fb1ee2022592de24bbae48352e17524
> URL:    http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=37812cef5fb1ee2022592de24bbae48352e17524
>
> Author: Peter Dunkley<peter.dunkley at crocodile-rcs.com>
> Committer: Peter Dunkley<peter.dunkley at crocodile-rcs.com>
> 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:   http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commitdiff;h=37812cef5fb1ee2022592de24bbae48352e17524
>
> _______________________________________________
> sr-dev mailing list
> sr-dev at lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20120411/0b559085/attachment-0001.htm>


More information about the sr-dev mailing list