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

[Henning Westerholt]

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