[Asterisk-code-review] chan pjsip: Creating Channel Causes Asterisk to Crash When D... (asterisk[master])

[Ashley Sanders]

Ashley Sanders has posted comments on this change.

Change subject: chan_pjsip: Creating Channel Causes Asterisk to Crash When Duplicate AOR Sections Exist in pjsip.conf
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.asterisk.org/#/c/254/1//COMMIT_MSG
Commit Message:

Line 7: chan_pjsip: Creating Channel Causes Asterisk to Crash When Duplicate AOR Sections Exist in pjsip.conf
> I'd try to knock this down to 80 characters or less. How about:
Sure thing - The wording as it is now, is the exact title of the issue, for consistency purposes, but I have no qualms making this shorter.


Line 7: chan_pjsip: Creating Channel Causes Asterisk to Crash When Duplicate AOR Sections Exist in pjsip.conf
      : 
      : This patch modifies the current loading strategy to consider the entire
      : pjsip configuration as invalid if duplicate sections (e.g. sections containing
      : the same [id/type]) are defined in pjsip.conf. If a duplicate section
      : is encountered during load, the entire configuration is rejected and destroyed,
      : an error message is logged and the load processing stops.
> As well - while this may have uncovered the bug it's not the bug in questio
Yes, this text is simply a description of what the patch actually accomplishes, without any context as to 'why'. I can add a statement to describe why this patch exists at all, if that would make the intent more clear.


https://gerrit.asterisk.org/#/c/254/1/res/res_sorcery_config.c
File res/res_sorcery_config.c:

Line 297: 		/* Confirm an object with this id does not already exist in the bucket. If so, the configuration is invalid so, destroy it and stop processing. */
> Granted, this may not be consistent everywhere, but I'd try to wrap the cod
I agree with you, however, the coding guidelines also dictate following the pattern previously established in the code.

Originally, I broke the pattern in favor of the shorter line length (to honor our coding guidelines) and it looked so different from the rest of the code, I changed my strategy to use the original style.


Line 299: 			ast_log(LOG_ERROR, "Config file '%s' could not be loaded; configuration contains a duplicate object: '%s' of type '%s'\n",
        : 				config->filename, id, type);
        : 			ast_config_destroy(cfg);
        : 			return;
> obj is a RAII_VAR which calls ao2_cleanup, that cleans it up
On line 236: RAII_VAR(struct ao2_container *, objects, NULL, ao2_cleanup);

The ao2_cleanup call is assigned as the function to invoke when the container goes out of scope.


-- 
To view, visit https://gerrit.asterisk.org/254
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I35090ca4cd40f1f34881dfe701a329145c347aef
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes