Re: [sr-dev] PKG/SHM allocation failure log macros

Am Freitag, 28. Dezember 2018, 20:47:59 CET schrieb Daniel-Constantin Mierla: Hi Daniel, #define helpers were added, I did a quick test with gcc (obviously not for suncc). Best regards, Henning > >> I noticed many commits replacing the log messages in case of allocation > >> failure with some macros. That is good, bringing consistency, but I > >> think that we should offer couple of them. The current one is rather dry > >> (meaning that it offers very few context details), which matches most of > >> the existing log messages used in such cases. > > > yes - I noticed that these error message were really inconsistent, > > sometimes even hard to understand for normal users (like "running out of > > pkg"). > > > This was my motivation to do the conversion. Additionally it is this way > > much easier to analyze the log files for this particular errors. > > >> But there are also other log messages for such cases which give more > >> details, like for what the allocation fails, some also giving the > >> requested size of allocation. > > > In the core this was done inconsistently (like some error msg were more > > detailed than others, sometimes in the same function). Sometimes the error > > message was wrong because of some changes to the code. Therefore I removed > > it in some cases. > > > For modules that allocate bigger memory blocks (htable, lcr etc..) I can > > see why it make sense to output more information. > > > As the core is using really small memory blocks, I don't think information > > about the size is really useful here. The usual response to this problem > > would be to just increase the PKG memory pool. > > > The location were the allocation error happens can be usually also spot > > from the logging message (the function name). > > >> So besides the current two macros (one for shm and one for pkg), we > >> should add few more. Like: > >> >> #define PKG_MEM_ERROR_MSG(m) LM_ERR("could not allocate private memory > >> from pkg pool - %s\n", m); > >> >> So one can do: > >> >> PKG_MEM_ERROR_MSG("needed for htable struct"); > >> >> And one to include also the size: > >> >> #define PKG_MEM_ERROR_SZ(s, m) LM_ERR("could not allocate private memory > >> from pkg pool - size: %u - %s\n", (unsigned int)s, m); > > > I will add macros for this two additional cases, probably this evening. > > >> No need to revert what was done, but I think for the future we would > >> preserve better information for troubleshooting in some cases, instead > >> of replacing those messages that now have more details with the bare > >> error log message. > > > See my comment above, in which conditions do you think we should preserve > > the size and additional error message during the conversation? > this is probably to be decided case by case. There are some log messages > giving even more context, like when building a list, it also prints the > value of the iterator, so it shows the failure happened when allocating > the Nth item in the list. > Thinking of that, maybe it would be better to add only one extra macro > instead of the two, that will allow to pass the formatting string to be > concatenated with the common log prefix and the arguments, so overall > will be two, one with a static message and the other allowing to add to > it, like: > #define PKG_MEM_ERROR LM_ERR("could not allocate ...\n") > #define PKG_MEM_ERROR_FMT(fmt, args...) LM_ERR("could not allocate ..." > fmt , ## args) > So one can do: > PKG_MEM_ERROR_FMT(" for the record index %d (size: %u)\n", i, > sizeof(struct)); > So there is a common prefix for all such errors, followed by whatever > the developer wants to add. > I think the PKG_MEM_ERROR_FMT has to be defined based on compiler, like > it is done for the LM_XYZ() inside core/dprint.h, because compilers use > different token for the extra arguments. -- Henning Westerholt - https://skalatan.de/blog/ Kamailio services - https://skalatan.de/services Kamailio security assessment - https://skalatan.de/de/assessment