[sr-dev] presence module (API)

[Richard Good]

Hi

The attached patch fixes RLS module use of pres_delete_shtable  - its
fairly straight forward as pres_delete_shtable is only used once and not
exported as I thought it might be.

If everyone is in agreement I will commit this change and the presence
module change to the master branch.

Regards
Richard.





24 January 2014 18:04, Andrew Mortensen <admorten at isc.upenn.edu> wrote:

>
> On Jan 24, 2014, at 10:54 AM, Richard Good <richard.good at smilecoms.com>
> wrote:
>
> > Hi
> >
> > The attached patch fixes our problem.
> >
> > As you suggested delete_shtable takes a subs_t pointer as an argument
> and compares the dialog's full tag set before deleting.
> >
> > If everyone is OK with this patch, on Monday I will look at a patch for
> the RLS module - it looks a little bit tricky as I think RLS exports the
> pres_delete_shtable through its own API as rls_delete_shtable, though I'm
> not 100% sure on this.
>
> Looks good to me. Thanks.
>
> andrew
>
>
>
> >
> > On 23 January 2014 17:24, Andrew Mortensen <admorten at isc.upenn.edu>
> wrote:
> >
> > On Jan 23, 2014, at 10:06 AM, Jason Penton <jason.penton at gmail.com>
> wrote:
> >
> > > Hey Andrew,
> > >
> > > Shot for the prompt response. I am happy to make these changes if we
> all agree... I was concerned like you of the impact on the consumers, but
> as you say just RLS.
> > >
> > > Can I go ahead?
> >
> > You have my support. Post the patch here for review.
> >
> > andrew
> >
> >
> >
> > >
> > > On Thu, Jan 23, 2014 at 5:00 PM, Andrew Mortensen <
> admorten at isc.upenn.edu> wrote:
> > >
> > > On Jan 23, 2014, at 9:19 AM, Jason Penton <jason.penton at gmail.com>
> wrote:
> > >
> > > > Hey all,
> > > >
> > > > I was taking a look at the presence module API code, specfically the
> API code for the hash table. I see that when we delete a subscription
> according to hash.c:
> > > > [snip]
> > > > Why are we only searching on to-tag? What if there is a collision on
> the hash AND the to-tag is the same for 2+ different subscriptions. This is
> even more likely of happening considering that the hash calculation is
> based only on the callid and to-tag...
> > >
> > > At least nominally the tag should be globally unique, per RFC3261:
> > >
> > > 19.3 ... When a tag is generated by a UA for insertion into a request
> or
> > >    response, it MUST be globally unique and cryptographically random
> > >    with at least 32 bits of randomness.
> > >
> > > But I agree that poorly behaved UAs could cause problems here. (We
> actually ran into duplicate to-tags recently with vendor handsets booted
> simultaneously.)
> > >
> > > It looks to me like we should be calling search_shtable, which
> compares the dialog's full tag set. The problem is that unlike other
> similar calls, delete_shtable doesn't take a subs_t pointer as an argument,
> so the only point of reference  is the to-tag. Fortunately it looks like
> there's only one place delete_shtable's called in the presence module, so
> it shouldn't be much work to start using search_shtable to find the
> subscription to delete.
> > >
> > > To complicate things further, though, delete_shtable is exported as
> part of the presence API, so we'd need to update all API consumers as well.
> That doesn't seem unreasonable, as a quick git grep indicates the only
> current user of the presence API is the rls module.
> > >
> > > andrew
> > > _______________________________________________
> > > sr-dev mailing list
> > > sr-dev at lists.sip-router.org
> > > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
> > >
> > > _______________________________________________
> > > sr-dev mailing list
> > > sr-dev at lists.sip-router.org
> > > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
> >
> >
> > _______________________________________________
> > sr-dev mailing list
> > sr-dev at lists.sip-router.org
> > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
> >
> > This email is subject to the disclaimer of Smile Communications at
> http://www.smilecoms.com/disclaimer
> >
> >
> > <presence_mod.diff>_______________________________________________
> > sr-dev mailing list
> > sr-dev at lists.sip-router.org
> > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>
>
> _______________________________________________
> sr-dev mailing list
> sr-dev at lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>

This email is subject to the disclaimer of Smile Communications at http://www.smilecoms.com/disclaimer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20140128/dcf6e7c0/attachment.html>
-------------- next part --------------
diff --git a/modules/rls/notify.c b/modules/rls/notify.c
index eb6a0e1..782a041 100644
--- a/modules/rls/notify.c
+++ b/modules/rls/notify.c
@@ -958,7 +958,7 @@ void rls_notify_callback( struct cell *t, int type, struct tmcb_params *ps)
 			/* delete from cache table */
 			hash_code= core_hash(&subs.callid, &subs.to_tag , hash_size);
 
-			if(pres_delete_shtable(rls_table,hash_code, subs.to_tag)< 0)
+			if(pres_delete_shtable(rls_table,hash_code, &subs)< 0)
 			{
 				LM_ERR("record not found in hash table\n");
 			}
diff --git a/modules/rls/rls.c b/modules/rls/rls.c
index 453b43e..0b9ccfa 100644
--- a/modules/rls/rls.c
+++ b/modules/rls/rls.c
@@ -119,7 +119,7 @@ extern void rls_destroy_shtable(shtable_t htable, int hash_size);
 extern int rls_insert_shtable(shtable_t htable,unsigned int hash_code, subs_t* subs);
 extern subs_t* rls_search_shtable(shtable_t htable,str callid,str to_tag,
 		str from_tag,unsigned int hash_code);
-extern int rls_delete_shtable(shtable_t htable,unsigned int hash_code,str to_tag);
+extern int rls_delete_shtable(shtable_t htable,unsigned int hash_code, subs_t* subs);
 extern int rls_update_shtable(shtable_t htable,unsigned int hash_code, 
 		subs_t* subs, int type);
 extern void rls_update_db_subs_timer(db1_con_t *db,db_func_t dbf, shtable_t hash_table,
diff --git a/modules/rls/rls_db.c b/modules/rls/rls_db.c
index 86303ad..d978a82 100644
--- a/modules/rls/rls_db.c
+++ b/modules/rls/rls_db.c
@@ -85,7 +85,7 @@ subs_t* rls_search_shtable(shtable_t htable,str callid,str to_tag,
 
 /******************************************************************************/
 
-int rls_delete_shtable(shtable_t htable,unsigned int hash_code,str to_tag)
+int rls_delete_shtable(shtable_t htable,unsigned int hash_code, subs_t* subs)
 {
   LM_ERR( "rls_delete_shtable shouldn't be called in RLS_DB_ONLY mode\n" );
   return(-1);