[PATCH] log: Add helpers for calling log_msg_ret() et al

Thu Apr 3 20:03:19 CEST 2025

Hi Simon,

On 4/3/25 7:57 PM, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 2 Apr 2025 at 06:19, Tom Rini <trini at konsulko.com> wrote:
>>
>> On Wed, Apr 02, 2025 at 04:48:35AM +1300, Simon Glass wrote:
>>> Hi Tom,
>>>
>>> On Mon, 31 Mar 2025 at 03:45, Tom Rini <trini at konsulko.com> wrote:
>>>>
>>>> On Fri, Mar 28, 2025 at 04:44:53AM -0600, Simon Glass wrote:
>>>>> Hi Tom,
>>>>>
>>>>> On Thu, 27 Mar 2025 at 17:50, Tom Rini <trini at konsulko.com> wrote:
>>>>>>
>>>>>> On Wed, Mar 26, 2025 at 10:46:57AM -0600, Simon Glass wrote:
>>>>>>> Hi Quentin,
>>>>>>>
>>>>>>> On Tue, 25 Mar 2025 at 04:20, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>>>>>>>>
>>>>>>>> Hi Simon,
>>>>>>>>
>>>>>>>> On 3/20/25 4:26 AM, Simon Glass wrote:
>>>>>>>>> Hi Quentin,
>>>>>>>>>
>>>>>>>>> On Wed, 19 Mar 2025 at 16:31, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Simon
>>>>>>>>>>
>>>>>>>>>> On 3/19/25 4:03 PM, Simon Glass wrote:
>>>>>>>>>>> Hi Quentin,
>>>>>>>>>>>
>>>>>>>>>>> On Wed, 19 Mar 2025 at 13:04, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Simon,
>>>>>>>>>>>>
>>>>>>>>>>>> On 3/19/25 12:49 PM, Simon Glass wrote:
>>>>>>>>>>>>> Logging of function return-values is used very frequently in U-Boot now.
>>>>>>>>>>>>> Add a few helper macros to make it less verbose to use.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It turns out that the log_ret() variants are not so useful, since it is
>>>>>>>>>>>>> not obviously where the error is coming from. So only the log_msg_ret()
>>>>>>>>>>>>> variants are worthy of these macros.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>>      include/log.h | 26 ++++++++++++++++++++++++++
>>>>>>>>>>>>>      1 file changed, 26 insertions(+)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/include/log.h b/include/log.h
>>>>>>>>>>>>> index 4f6d6a2c2cf..bdda7af570c 100644
>>>>>>>>>>>>> --- a/include/log.h
>>>>>>>>>>>>> +++ b/include/log.h
>>>>>>>>>>>>> @@ -380,6 +380,32 @@ void __assert_fail(const char *assertion, const char *file, unsigned int line,
>>>>>>>>>>>>>      #define log_msg_retz(_msg, _ret) ((void)(_msg), _ret)
>>>>>>>>>>>>>      #endif
>>>>>>>>>>>>>
>>>>>>>>>>>>> +/*
>>>>>>>>>>>>> + * LOGR() - helper macro for calling a function and logging error returns
>>>>>>>>>>>>> + *
>>>>>>>>>>>>> + * Logs if the function returns a negative value
>>>>>>>>>>>>> + *
>>>>>>>>>>>>> + * Usage:   LOGR("abc", my_function(...));
>>>>>>>>>>>>> + */
>>>>>>>>>>>>> +#define LOGR(_msg, _expr)    do {            \
>>>>>>>>>>>>> +     int _ret = _expr;                       \
>>>>>>>>>>>>> +     if (_ret < 0)                           \
>>>>>>>>>>>>> +             return log_msg_ret(_msg, _ret); \
>>>>>>>>>>>>> +     } while (0)
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +/*
>>>>>>>>>>>>> + * LOGZ() - helper macro for calling a function and logging error returns
>>>>>>>>>>>>> + *
>>>>>>>>>>>>> + * Logs if the function returns a non-zero value
>>>>>>>>>>>>> + *
>>>>>>>>>>>>> + * Usage:   LOGZ("abc", my_function(...));
>>>>>>>>>>>>> + */
>>>>>>>>>>>>> +#define LOGZ(_msg, _expr)    do {            \
>>>>>>>>>>>>> +     int _ret = _expr;                       \
>>>>>>>>>>>>> +     if (_ret)                               \
>>>>>>>>>>>>> +             return log_msg_retz(_msg, _ret);        \
>>>>>>>>>>>>> +     } while (0)
>>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>> Mmmm not sure this forced return call is a good idea, this would forbid
>>>>>>>>>>>> us from performing some unwinding for example.
>>>>>>>>>>>
>>>>>>>>>>> Yes, it is really only for simple cases. Without the return, there
>>>>>>>>>>> isn't much value and you may as well not use this macro.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I don't really see how that is better than simply calling
>>>>>>>>>>>>
>>>>>>>>>>>> return log_msg_retz("abc", my_function());
>>>>>>>>>>>
>>>>>>>>>>> That's not the intention. It actually replaces:
>>>>>>>>>>>
>>>>>>>>>>> ret = my_function();
>>>>>>>>>>> if (ret)
>>>>>>>>>>>        return_log_msg_ret("abc", ret);
>>>>>>>>>>>
>>>>>>>>>>> I use this a lot in my code. You can't always just return, since there
>>>>>>>>>>> may not be an error.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I see, I read too fast again. Only return if it's an error.
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ?
>>>>>>>>>>>>
>>>>>>>>>>>> If we were to keep this, I would recommend to rename the macro and fix
>>>>>>>>>>>> the docstring because it does not only log if the function returns a
>>>>>>>>>>>> non-zero value. It does actually return.
>>>>>>>>>>>>
>>>>>>>>>>>> So something like
>>>>>>>>>>>>
>>>>>>>>>>>> LOGZ_AND_RETURN(_msg, _expr)
>>>>>>>>>>>>
>>>>>>>>>>>> maybe?
>>>>>>>>>>>
>>>>>>>>>>> Sure, longer names are easier to learn, but then it is so long I doubt
>>>>>>>>>>> anyone would use it.
>>>>>>>>>>>
>>>>>>>>>>> Perhaps LOG_RET() and LOG_RETZ() ? But they might get confused with
>>>>>>>>>>> log_ret() and log_retz(), which I am actually thinking of deleting.
>>>>>>>>>>>
>>>>>>>>>>> I would like the shortest possible name to avoid spilling functions
>>>>>>>>>>> onto the next line all the time.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It should be absolutely obvious from the macro name that this can in
>>>>>>>>>> fact return because missing to unwind code is among the things
>>>>>>>>>> developers typically easily miss already, so with this macro it'll be
>>>>>>>>>> even easier to forget about undoing things.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yes that's true. We don't have a huge amount of tests for this 'undo'
>>>>>>>>> code either. I would bet that a code-coverage map would show that.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yeah but that's not a reason to make it even harder to spot issues in
>>>>>>>> the undo code :)
>>>>>>>
>>>>>>> I suspect people will just have to learn the macros, as they have with
>>>>>>
>>>>>> I would ask "What kernel construct have people already learned and we
>>>>>> can adapt inside the macro?".
>>>>>
>>>>> Is there such a thing?
>>>>
>>>> Likely so. This isn't some new and novel problem, and it's likely more
>>>> people have put thought in to this and come up with something well
>>>> reviewed there.
>>>
>>> What are you referring to here? I am not seeing anything in Linux
>>> related to this.
>>
>> Then it's probably more pain than help in getting everyone to write code
>> that handles wind/unwind/logging consistently and correctly and no we
>> shouldn't wrap this up in some macro.
> 
> I don't have my heart set on this patch. Having used it in quite a bit
> of code I think it has value, but it has drawbacks too.
> 
> Quentin, what do you think?
> 

I don't think the balance between brevity and potential confusion is 
appropriate here, I would prefer not to have this.

Cheers,
Quentin