| Message ID | 20231129192106.1295868-2-gerickson@nuovations.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/8] log: Refactor debugging preprocessor macros. | expand |
Hi Grant, > This refactors the debugging / debug log preprocessor-related > macros. In particular, it: > > * Introduces CONNMAN_DEBUG_DESC_INSTANTIATE which declares and > instantiates an instance of 'connman_debug_desc'. > > * Replaces CONNMAN_DEBUG_DEFINE with CONNMAN_DEBUG_ALIAS which > declares and instantiates an alias (that is, asserts the > CONNMAN_DEBUG_FLAG_ALIAS flag) instance of > 'connmand_debug_desc', using CONNMAN_DEBUG_DESC_INSTANTIATE. > > * Redefines 'DBG' against 'CONNMAN_DEBUG_DESC_INSTANTIATE' with > '__func__' as the function parameter. > --- > include/log.h | 28 +++++++++++++++++----------- > src/log.c | 2 +- > 2 files changed, 18 insertions(+), 12 deletions(-) I am bit confused. I thought we leave this out for now and get the fixes merged and look into improving DBG. Regards Marcel
On Nov 30, 2023, at 12:39 AM, Marcel Holtmann <marcel@holtmann.org> wrote: >> This refactors the debugging / debug log preprocessor-related >> macros. In particular, it: >> >> * Introduces CONNMAN_DEBUG_DESC_INSTANTIATE which declares and >> instantiates an instance of 'connman_debug_desc'. >> >> * Replaces CONNMAN_DEBUG_DEFINE with CONNMAN_DEBUG_ALIAS which >> declares and instantiates an alias (that is, asserts the >> CONNMAN_DEBUG_FLAG_ALIAS flag) instance of >> 'connmand_debug_desc', using CONNMAN_DEBUG_DESC_INSTANTIATE. >> >> * Redefines 'DBG' against 'CONNMAN_DEBUG_DESC_INSTANTIATE' with >> '__func__' as the function parameter. >> --- >> include/log.h | 28 +++++++++++++++++----------- >> src/log.c | 2 +- >> 2 files changed, 18 insertions(+), 12 deletions(-) > > I am bit confused. I thought we leave this out for now and get the fixes merged and look into improving DBG. As we last discussed on the thread: >> CONNMAN_DEBUG aside, are you otherwise OK with the clean ups and documentation to log.h embodied in this patch? > > I am fine with all the cleanups, but I prefer you don’t introduce CONNMAN_DEBUG and stick with DBG instead and we try to figure out on how to make DBG work the way you want to use it. As discussed and agreed, I dropped the CONNMAN_DEBUG macro from that part of the patch set and simply documented the existing macros and eliminated the copy-and-paste by preserving the CONNMAN_DEBUG_DESC_INSTANTIATE macro. So, as preferred, everything that follows remains exclusively with DBG and DBG only. I remain happy to loop back and improve DBG at a later time. Specifically, for the “called by function” use case of DBG that we discussed, there are a healthy number of existing use cases in the code base that would benefit, the most notable of which are the ‘*_{ref,unref}_debug’ functions used for reference counting tracking and debugging (the whole reference counting pattern seems replicated, ideally it could all be embodied in infrastructure written once and instantiated where necessary). Does that help clarify? Best, Grant
diff --git a/include/log.h b/include/log.h index 8b00e9dc979c..a9b0f17f392b 100644 --- a/include/log.h +++ b/include/log.h @@ -58,11 +58,17 @@ struct connman_debug_desc { unsigned int flags; } __attribute__((aligned(8))); -#define CONNMAN_DEBUG_DEFINE(name) \ - static struct connman_debug_desc __debug_alias_ ## name \ +#define CONNMAN_DEBUG_DESC_INSTANTIATE(symbol, _name, _file, _flags) \ + static struct connman_debug_desc symbol \ __attribute__((used, section("__debug"), aligned(8))) = { \ - #name, __FILE__, CONNMAN_DEBUG_FLAG_ALIAS \ - }; + .name = _name, .file = _file, .flags = _flags \ + } + +#define CONNMAN_DEBUG_ALIAS(suffix) \ + CONNMAN_DEBUG_DESC_INSTANTIATE(__debug_alias_##suffix, \ + #suffix, \ + __FILE__, \ + CONNMAN_DEBUG_FLAG_ALIAS) /** * DBG: @@ -73,13 +79,13 @@ struct connman_debug_desc { * name it is called in. */ #define DBG(fmt, arg...) do { \ - static struct connman_debug_desc __connman_debug_desc \ - __attribute__((used, section("__debug"), aligned(8))) = { \ - .file = __FILE__, .flags = CONNMAN_DEBUG_FLAG_DEFAULT, \ - }; \ - if (__connman_debug_desc.flags & CONNMAN_DEBUG_FLAG_PRINT) \ - connman_debug("%s:%s() " fmt, \ - __FILE__, __FUNCTION__ , ## arg); \ + CONNMAN_DEBUG_DESC_INSTANTIATE(__connman_debug_desc, \ + 0, \ + __FILE__, \ + CONNMAN_DEBUG_FLAG_DEFAULT); \ + if (__connman_debug_desc.flags & CONNMAN_DEBUG_FLAG_PRINT) \ + connman_debug("%s:%s() " fmt, \ + __FILE__, __func__, ##arg); \ } while (0) #ifdef __cplusplus diff --git a/src/log.c b/src/log.c index 554b046b07fa..f7483194b230 100644 --- a/src/log.c +++ b/src/log.c @@ -38,7 +38,7 @@ static const char *program_exec; static const char *program_path; /* This makes sure we always have a __debug section. */ -CONNMAN_DEBUG_DEFINE(dummy); +CONNMAN_DEBUG_ALIAS(dummy); /** * connman_info:
This refactors the debugging / debug log preprocessor-related macros. In particular, it: * Introduces CONNMAN_DEBUG_DESC_INSTANTIATE which declares and instantiates an instance of 'connman_debug_desc'. * Replaces CONNMAN_DEBUG_DEFINE with CONNMAN_DEBUG_ALIAS which declares and instantiates an alias (that is, asserts the CONNMAN_DEBUG_FLAG_ALIAS flag) instance of 'connmand_debug_desc', using CONNMAN_DEBUG_DESC_INSTANTIATE. * Redefines 'DBG' against 'CONNMAN_DEBUG_DESC_INSTANTIATE' with '__func__' as the function parameter. --- include/log.h | 28 +++++++++++++++++----------- src/log.c | 2 +- 2 files changed, 18 insertions(+), 12 deletions(-)