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

[Simon Glass]

Hi Quentin,

On Fri, 4 Apr 2025 at 07:03, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> 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.

OK, I'll drop it.

Regards,
Simon