< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

Re: [reSIProcate] upcoming changes to DUM's RegistrationPersistenceManager and ServerRegistration


Overall, the proposal looks sound to me. A few minor comments inline.

Also, I know that David Bryan has been doing some investigation into how to modify these data structures so that they have hooks to support storage of P2P information in registration records. You probably want to talk with him to determine if there are some things you can do in the refactoring to make such changes easier and/or possible.

Rohan Mahy wrote:


class BaseContactRecord {
   NameAddr contact;    // can contain callee caps
   time_t regExpires;
   time_t lastUpdated;
   Tuple receivedFrom;  // source transport, IP address, and port
   UriList sipPath;     // Value of SIP Path header from the Request
};


Does the NameAddr contain the GRUU information also? That is, is this going to be an exact copy of what comes off the wire, or do we have to tear it apart and resynthesize it? If it's the latter, then I think I like the other approach -- of storing the actual capabilities as a separate item -- better. (In fact, it might even be better in the former case). I'm thinking of how I would go about implementing a search for the correct contact based on caller prefs; it would be much faster if the callee caps were already broken down and pre-digested in an appropriate structure (e.g. a hash of capabilities).

class Aor {
   Uri aor;
   SimpleContactList contacts;
   InstanceList instances;
   UriList aliases;
};


I suppose that the "aliases" field here is used to remove aliases when deregistration occurs, and nothing else, right?



2. ServerRegistration::dispatch first processes a registration by first locking then adding, updating, or deleting records in the "database", then if reject is called, we roll back the changes. accept() and reject() unlock the database. This is problematic for a number of reasons. This is a DoS bait if I ever saw it. Sending a lot of illegitimate REGISTERs can lock the database and prevent the LocationServer from accessing the locked records.


When I did this, I intended to eventually overhaul it to be more like a two-phase commit, where the database could confirm the update before effecting it, but the application would have the opportunity to enforce policy before readers had access. It was a Known Ugly Hack just to get things up and running. :)

- We don't want Registrations to impact the actual database unless they are actually accepted - We should lock the database as briefly as possible. It would be great if we only needed write locks (we could do this if all the operations on the database were atomic)*


It's completely unclear to me how you can make reads through STL structures atomic. I think you'll probably still need AOR-level locking here. Read/Write locks make much more sense than mutexes.

I propose refactoring ServerRegistration so we pass a list of operations (memory owned by DUM) in the ServerRegistrationHandlers. The application can inspect the operations and/or the original request. accept() would cause the list of operations to be committed to the database (quick lock/unlock), return the current list of registered contacts (with and without instances), and free the list of operations. reject() would free the memory.


That sounds like a good design to me.

We can use the same operations for updates from other cluster members when we add high availability.


Yep. I like that.

/a