Hello,

On 28.12.18 16:37, Victor Seva wrote:

Hello,

On Fri, 28 Dec 2018 at 16:22, Daniel-Constantin Mierla <miconda@gmail.com mailto:miconda@gmail.com> wrote:

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.

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.

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);

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.

I'm not sure about the benefits of adding a custom message but I see that having the requested size in the log maybe can be useful in case of a mistake in the (re)allocation. Do you think that if the pool is exhausted, makes sense to know, with a custom log, where the reservation failed?

The custom message is useful to avoid going to source code to figure out where it fails inside same function. There can be couple of allocations inside the same function and when one runs an older version and reports some errors it can happen that the current head of that branche has different lines for those allocations, so it would require to check out the exact snapshot. However, sometime that is no longer known if it is running a build from a tarball that doesn't have the git commit id. Also, I do not remember if deb/rpm packages put commit id inside the version string. But I remember I faced couple of situations like these in the past and decided to add more context in the error message, even if it was still a static string.

Cheers, Daniel

Hello,

On 28.12.18 16:38, Henning Westerholt wrote:

Am Freitag, 28. Dezember 2018, 16:21:59 CET schrieb Daniel-Constantin Mierla:

...

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.

Hi Daniel,

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.

Cheers, Daniel