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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5053 bytes --]

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@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@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@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@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?".

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]