----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3254/#review11075 -----------------------------------------------------------
Very cool functionality. Could use a mention in CHANGES :-) branches/12/funcs/func_sorcery.c <https://reviewboard.asterisk.org/r/3254/#comment20717> Throw a space between <= and 0 branches/12/main/config.c <https://reviewboard.asterisk.org/r/3254/#comment20718> In your fast forward loops, put spaces between the assignments: curr = curr->next branches/12/main/sorcery.c <https://reviewboard.asterisk.org/r/3254/#comment20719> It took me a bit to realize that _vl was "variable list" and not "version 1" (silly fonts...) The various functions that use that nomenclature, it may be worthwhile spelling that out as "var_list" branches/12/res/res_pjsip/location.c <https://reviewboard.asterisk.org/r/3254/#comment20720> Just so you don't have to check for another memory allocation failure here (as ast_strdup can fail), you could write this instead as: if (strcmp(camel, "Contact") == 0) { ast_free(camel); camel = NULL; } ast_str_append(buf, 0, "%s: %s\r\n", S_OR(camel, "Contacts"), i->value); That would also catch the case where i->name (somehow?) is "". branches/12/res/res_pjsip/location.c <https://reviewboard.asterisk.org/r/3254/#comment20721> ast_strdup could theoretically fail. It'd be worthwhile to return failure if it did. branches/12/tests/test_sorcery.c <https://reviewboard.asterisk.org/r/3254/#comment20722> Hooray unit tests! branches/12/tests/test_sorcery.c <https://reviewboard.asterisk.org/r/3254/#comment20723> Since this test now also covers AST_SORCERY, go ahead and put a module depends on it in the XML at the top of the file: /*** MODULEINFO <depend>func_sorcery</depend> ***/ - Matt Jordan On Feb. 28, 2014, 3:27 p.m., George Joseph wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3254/ > ----------------------------------------------------------- > > (Updated Feb. 28, 2014, 3:27 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-22537 > https://issues.asterisk.org/jira/browse/ASTERISK-22537 > > > Repository: Asterisk > > > Description > ------- > > This patch creates the AST_SORCERY dialplan function which allows someone to > retrieve any value from a sorcery-based config file. It's similar to > AST_CONFIG. > > The creation of the function itself was fairly straightforward but it > required changes to the underlying sorcery infrastructure that rippled into > individual sorcery objects. The changes stemmed from inconsistencies in how > sorcery created ast_variable objectsets from sorcery objects and the > inconsistency in how individual objects used that feature especially when it > came to parameters that can be specified multiple times like contact in aor > and match in identify. You can read more here... > http://lists.digium.com/pipermail/asterisk-dev/2014-February/065202.html > > So, what this patch does, besides actually creating the AST_SORCERY function, > is the following... > > Creates ast_variable_list_append which is a helper to append one ast_variable > list to another. > Modifies the ast_sorcery_object_field_register functions to accept the > already-defined sorcery_fields_handler callback. > Modifies ast_sorcery_objectset_create to accept a parameter indicating return > type preference...a single ast_variable with all values concatenated or an > ast_variable list with multiple entries. Also fixed a few bugs. > Modifies individual sorcery object implementations to use the new function > definition of the ast_sorcery_object_field_register functions. > Modifies location.c and res_pjsip_endpoint_identifier_ip.c to implement > sorcery_fields_handler handlers so they return multiple occurrences as an > ast_variable_list. > Added a whole bunch of tests to test_sorcery. > > NOTES: > > If you read through the email chain, I had a question on whether we > should/could change "Contacts" to "Contact" in AMI output. Joshua though we > should, which I agree with, but there is the issue of backward compatibility. > What is did therefore was to put a little translation code in to keep it > plural for 12, then if you guys agree, remove that code in trunk/13 so it's > singular. I'll have to do a new patch for trunk anyway because > ast_sip_auth_array changed to a vector in trunk. > > > Diffs > ----- > > branches/12/tests/test_sorcery.c 409132 > branches/12/res/res_pjsip_outbound_registration.c 409132 > branches/12/res/res_pjsip_endpoint_identifier_ip.c 409132 > branches/12/res/res_pjsip_acl.c 409132 > branches/12/res/res_pjsip/pjsip_configuration.c 409132 > branches/12/res/res_pjsip/location.c 409132 > branches/12/res/res_pjsip/config_transport.c 409132 > branches/12/res/res_pjsip/config_auth.c 409132 > branches/12/main/sorcery.c 409132 > branches/12/main/config.c 409132 > branches/12/main/bucket.c 409132 > branches/12/include/asterisk/sorcery.h 409132 > branches/12/include/asterisk/config.h 409132 > branches/12/funcs/func_sorcery.c PRE-CREATION > > Diff: https://reviewboard.asterisk.org/r/3254/diff/ > > > Testing > ------- > > Created a new test in test_sorcery for the dialplan function which has the > side effect of also checking the changes to sorcery for multiple values. > Ran testsuite tests/channels/pjsip to check sorcery functions and AMI output. > > *CLI> test execute category /main/sorcery/ > Running all available tests matching category /main/sorcery/ > START /main/sorcery/ - dialplan_function > END /main/sorcery/ - dialplan_function Time: <1ms Result: PASS > START /main/sorcery/ - configuration_file_wizard_retrieve_multiple_all > <snip> > 44 Test(s) Executed 44 Passed 0 Failed > > > testsuite$ python -u ./runtests.py -t tests/channels/pjsip | ./pretty_print > Running tests for Asterisk 12.1.0 > Test > Run/Pass/Fail [ Status ] > tests/channels/pjsip/handle_options_request > 1/ 1/ 0 [ Passed ] > tests/channels/pjsip/incoming_calls_without_auth > 2/ 2/ 0 [ Passed ] > <snip> > tests/channels/pjsip/ami/show_endpoints > 57/ 57/ 0 [ Passed ] > tests/channels/pjsip/ami/show_endpoint > 58/ 58/ 0 [ Passed ] > tests/channels/pjsip/ami/show_registrations_inbound > 59/ 59/ 0 [ Passed ] > tests/channels/pjsip/ami/show_registrations_outbound > 60/ 60/ 0 [ Passed ] > tests/channels/pjsip/ami/show_subscriptions > 61/ 61/ 0 [ Passed ] > tests/channels/pjsip/set_var > 62/ 62/ 0 [ Passed ] > tests/channels/pjsip/hold/run-test > 63/ 62/ 1 [ Failed ] > tests/channels/pjsip/presence_pidf > 64/ 63/ 1 [ Passed ] > tests/channels/pjsip/presence_xpidf > 65/ 64/ 1 [ Passed ] > tests/channels/pjsip/mwi > 66/ 65/ 1 [ Passed ] > Tests: 66 Passed: 65 Failed: 1 > FAILED: tests/channels/pjsip/hold/run-test > > The failed one was failing before any changes. Not sure why. > > > Thanks, > > George Joseph > >
-- _____________________________________________________________________ -- 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
