> On April 2, 2015, 2:41 p.m., Mark Michelson wrote: > > /trunk/main/db.c, lines 958-977 > > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73358#file73358line958> > > > > The dialplan function allows an omitted type to automatically be > > interpreted as "global" but the CLI command requires that the type is > > present. > > > > I'm not a fan of the inconsistency. My preference would be that the > > dialplan function is changed to require an explicit type.
Works for me. > On April 2, 2015, 2:41 p.m., Mark Michelson wrote: > > /trunk/main/db.c, lines 419-441 > > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73358#file73358line419> > > > > There is potential for deadlock due to lock inversion. > > > > When ast_db_put_shared() is called, it locks the shared_families > > container and then calls into db_put_common(), which locks the dblock. > > > > When ast_db_put() is called, it first calls into db_put_common(), which > > locks the dblock. Since ast_db_put() sets the "shared" parameter to 1 when > > calling db_put_common(), db_put_common() will call into > > db_entry_put_shared(), which will then try to lock the shared_families lock. > > > > Probably the easiest way to go here would be to unlock shared_families > > before calling db_put_common(), establishing a locking order of dblock and > > then shared_families. Thanks for catching this! I think that locking order would be fine - and there's no real danger in unlocking the shared_families container after you've put the family in the container but before updating the key. Even if something else sneaks in and removes the shared family, we've already committed to changing the value. > On April 2, 2015, 2:41 p.m., Mark Michelson wrote: > > /trunk/main/db.c, lines 1674-1677 > > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73358#file73358line1674> > > > > family can't be NULL here since whatever strchr returns has 1 added to > > it. This really should be: family = strchr(cur->key + 1, '/'); if (!family) { continue; } family++; > On April 2, 2015, 2:41 p.m., Mark Michelson wrote: > > /trunk/tests/test_db.c, lines 62-63 > > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73363#file73363line62> > > > > Any reason you went with a manually-resized array instead of an > > AST_VECTOR? The code to synchronize and handle Stasis messages already had a template for it in another set of unit tests, so this was just adapted from that. > On April 2, 2015, 2:41 p.m., Mark Michelson wrote: > > /trunk/tests/test_db.c, lines 427-446 > > <https://reviewboard.asterisk.org/r/4490/diff/2/?file=73363#file73363line427> > > > > If any of these calls to ast_test_validate() fails the > > GLOBAL_SHARED_FAMILY and UNIQUE_SHARED_FAMILY will not be cleaned up. > > > > Actually, this pattern is common throughout the test code and should be > > addressed. RAII_VAR to the rescue. - Matt ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4490/#review15032 ----------------------------------------------------------- On March 29, 2015, 8:46 p.m., Matt Jordan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/4490/ > ----------------------------------------------------------- > > (Updated March 29, 2015, 8:46 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-24909 > https://issues.asterisk.org/jira/browse/ASTERISK-24909 > > > Repository: Asterisk > > > Description > ------- > > As described on the asterisk-dev mailing list [1], I got some inspiration > from seeing Kamailio's htable implementation, and thought a similar mechanism > would work for the Asterisk Database. This patch is the result. > > This patch provides a mechanism to mark families within the AstDB as > "shared." The marking of a family as shared is independent of the existance > of the family, and is independent of the updates already made to the family. > Shared families are subject to distribution with other Asterisk instances, as > well as subject to updates from other Asterisk instances. Two strategies for > sharing are implemented: > > * Global: A 'global' shared family shares the family/key space with all other > Asterisk instances. Say we have shared family 'foo', and we have two Asterisk > instances. Say the first Asterisk instance (ast1) updates a key in family > 'foo' to the following: > > ast1 > /foo/key = bar > > The second Asterisk instance (ast2) would then receive an update in its AstDB: > > ast2 > /foo/key = bar > > If ast2 later updates the same key in its local AstDB to 'foobar', ast1 will > receive a notification to update the same key in its AstDB: > > ast2 > /foo/key = foobar > ast1 > /foo/key = foobar > > * Unique: A 'unique' shared family shares its values with other Asterisk > instances, however, updates from other Asterisk instances are placed in > unique families in the local AstDB for each Asterisk instance. Again, say we > have shared family 'foo', and we have two Asterisk instances - ast1 and ast2. > ast1 has an EID of 11:11:11:11:11:11, while ast2 has an EID of > ff:ff:ff:ff:ff:ff. Say ast1 updates a key in family 'foo': > > ast1 > /foo/key = bar > > ast2 would receive the update for family 'foo', but instead of updating its > local copy, it would instead store the value in a new family for ast1 > corresponding to its EID: > > ast2 > /11:11:11:11:11:11/foo/key = bar > > If ast2 later updates the same ky in its local AstDB to 'foobar', the > received value from ast1 will not be updated. However, ast1 will receive the > update, and store the value in a new family for ast2 corresponding to its EID: > > ast2 > /foo/key = foobar > /11:11:11:11:11:11/foo/key = bar > ast1 > /foo/key = bar > /ff:ff:ff:ff:ff:ff/foo/key = foobar > > In order to manipulate shared families, two new dialplan functions have been > added, DB_SHARED and DB_SHARED_EXISTS. DB_SHARED allows for creation of a > shared family, as well as deletion, while DB_SHARED_EXISTS returns whether or > not a family is shared: > same => n,Set(DB_SHARED(put,global)=foo) ; share family 'foo' globally > same => n,Set(DB_SHARED(put,unique)=foobar) ; share family 'foobar' > uniquely > same => n,NoOp(${DB_SHARED_EXISTS(foo)}) ; returns '1' > same => n,Set(DB_SHARED(delete)=foo) ; remove shared family status > for 'foo' > > CLI commands were also added to create/delete shared families, and the output > of 'database show|showkey' updated to show the shared status of a > family/key/value tuple. > > Finally, a mechanism for sharing AstDB information was added to the PJSIP > stack's res_pjsip_publish_asterisk. This includes a new event type, > 'asterisk-db', which contains the values being created/deleted. Necessary > configuration parameters were added to the existing configuration objects > that support inbound/outbound PUBLISH support. An example of a PUBLISH > request with the new event type is shown below: > > PUBLISH sip:ast1@127.0.0.1:5061 SIP/2.0 > Via: SIP/2.0/UDP > 127.0.0.1:5060;rport;branch=z9hG4bKPj1eb182a7-a0aa-4d73-995d-d2ad4b096db2 > From: <sip:ast1@127.0.0.1>;tag=7b294642-06ae-4ecf-8637-db8ba2dc4397 > To: <sip:ast1@127.0.0.1> > Call-ID: b9463adc-e364-440d-8ce1-842372813b08 > CSeq: 48111 PUBLISH > Event: asterisk-db > Expires: 3600 > Max-Forwards: 70 > User-Agent: Asterisk PBX SVN-mjordan-trunk-astdb-cluster-URL:-r432916M-/trunk > Content-Type: application/json > Content-Length: 156 > > {"type":"dbstate","eid":"11:11:11:11:11:11","dbstate":{"verb":"put","family":"global_shared","share_type":"global","entries":[{"data":"foo","key":"key1"}]}} > > > As a note on the power of the frameworks in Asterisk 13 - in this case, both > Stasis and PJSIP - the vast bulk of this was written on two plane flights, > plus a weekend or so of test writing and cleanup. > > [1] http://lists.digium.com/pipermail/asterisk-dev/2015-March/073192.html > > > Diffs > ----- > > /trunk/tests/test_db.c 433716 > /trunk/res/res_pjsip_pubsub.c 433716 > /trunk/res/res_pjsip_publish_asterisk.c 433716 > /trunk/res/res_pjsip_outbound_publish.c 433716 > /trunk/main/utils.c 433716 > /trunk/main/db.c 433716 > /trunk/include/asterisk/utils.h 433716 > /trunk/include/asterisk/astdb.h 433716 > /trunk/funcs/func_db.c 433716 > /trunk/configs/samples/pjsip.conf.sample 433716 > /trunk/CHANGES 433716 > > Diff: https://reviewboard.asterisk.org/r/4490/diff/ > > > Testing > ------- > > Existing AstDB tests execute successfully, as do the existing PJSIP PUBLISH > tests. In addition: > > * Unit tests were written (included in this review) that verify the API > additions to the AstDB. This includes: > - Verification of creation/delation of shared families > - Verification that creating/deleting an AstDB entry in a shared family > publishes the correct Stasis message > - Verification that all keys/values in all shared families can be published > via a 'refresh' > > * Asterisk Test Suite tests for the PJSIP PUBLISH distribution of the AstDB > information are available at: https://reviewboard.asterisk.org/r/4508/ > > > Thanks, > > Matt Jordan > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
