[Asterisk-code-review] Logging: Add debug logging categories (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/14887 )

Change subject: Logging: Add debug logging categories
......................................................................


Patch Set 5:

(3 comments)

https://gerrit.asterisk.org/c/asterisk/+/14887/5/include/asterisk/logger_category.h 
File include/asterisk/logger_category.h:

https://gerrit.asterisk.org/c/asterisk/+/14887/5/include/asterisk/logger_category.h@97 
PS5, Line 97: const char *name
> Should this be the ID?  Not sure.
"name" is correct, as it does the look up by name. One place it is called from is "ast_debug_category_set_sublevels" which takes a list of names and passes the individual names to this function.


https://gerrit.asterisk.org/c/asterisk/+/14887/5/include/asterisk/logger_category.h@119 
PS5, Line 119: const char * const *names, size_t size
> How about making this a ast_vector_const_string?  It'd be easier to work with.
The reason I didn't do it that way is because this function is currently only called from the "core set debug categories ..." CLI command, which uses an array of strings. And it abstracts iterating over that array, so if say in the future verbose categories were added this function could be reused.

If this took a vector much of the functionality here would then need to be pushed up into the CLI command function. The array would be iterated and parsed there, strings copied, and then the vector iterated again here.

A vector version could be added in the future if something else ends up needing similar functionality but it was easier to use a vector in that case.


https://gerrit.asterisk.org/c/asterisk/+/14887/5/main/logger_category.c 
File main/logger_category.c:

https://gerrit.asterisk.org/c/asterisk/+/14887/5/main/logger_category.c@42 
PS5, Line 42: static uintmax_t get_next_id(struct categories_level_t *level)
            : {
            : 	if (level->id_pool == 0) {
            : 		level->id_pool = 1;
            : 	} else if (level->id_pool >= (UINTMAX_MAX / 2)) {
            : 		/* No more ids left*/
            : 		return 0;
            : 	} else {
            : 		level->id_pool <<= 1;
            : 	}
            : 
            : 	return level->id_pool;
            : }
            : 
            : static int cmp_by_name(const struct category_t *category, const char *name)
            : {
            : 	return !strcmp(category->name, name);
            : }
            : 
            : static uintmax_t category_register(struct categories_level_t *level, const char *name)
            : {
            : 	int i;
            : 	struct category_t *category;
            : 
            : 	AST_VECTOR_RW_WRLOCK(&level->categories);
            : 
            : 	i = AST_VECTOR_GET_INDEX(&level->categories, name, cmp_by_name);
            : 	if (i >= 0) {
            : 		AST_VECTOR_RW_UNLOCK(&level->categories);
            : 		ast_log(LOG_ERROR, "Cannot register logger category '%s'. "
            : 				"Name already used for type.\n", name);
            : 		return 0;
            : 	}
            : 
            : 	category = ast_calloc(1, sizeof(*category) + strlen(name) + 1);
            : 	if (!category) {
            : 		AST_VECTOR_RW_UNLOCK(&level->categories);
            : 		return 0;
            : 	}
            : 
            : 	category->id = get_next_id(level);
            : 	category->sublevel = AST_LOG_CATEGORY_DISABLED;
            : 	strcpy(category->name, name); /* Safe */
            : 
            : 	if (AST_VECTOR_APPEND(&level->categories, category)) {
            : 		AST_VECTOR_RW_UNLOCK(&level->categories);
            : 		ast_log(LOG_ERROR, "Cannot register logger category '%s'. "
            : 				"Unable to append.\n", name);
            : 		return 0;
            : 	}
            : 
            : 	AST_VECTOR_RW_UNLOCK(&level->categories);
            : 	ast_log(LOG_VERBOSE, "##### %s - %ju\n", category->name, category->id);
            : 	return category->id;
            : }
            : 
> I'm a little confused. […]
Ids must be a power of 2. This allows for fast lookup, and "or'ing" of ids in order to permit multiple categories in a log statement.

Using a vector would require a for loop on every category log statement called. It would also make specifying multiple categories for a given log statement a little awkward.

I'll add a comment in the code.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/14887
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I6e6cb247bb1f01dbf34750b2cd98e5b5b41a1849
Gerrit-Change-Number: 14887
Gerrit-PatchSet: 5
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Tue, 15 Sep 2020 15:33:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200915/ac7d0dd2/attachment.html>