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

[George Joseph]

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

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


Patch Set 5: Code-Review-1

(3 comments)

After looking at this more, I'm wondering if the ids need to be part of a bitfield.  If the id is the index into a vector then it's just as fast to check as if it were a bit in a uint64 and you don't have to deal with the trying to find an open bit.  Once you register and get an id, the id should be used for all subsequent calls.  That also spares you from having to search the vector by name.

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.


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.


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.  Why not just return the slot in the vector as the id?



-- 
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: Thu, 10 Sep 2020 16:02:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200910/7ea1a8ad/attachment.html>