Hi Quentin,
On Wed, 19 Mar 2025 at 16:31, Quentin Schulz <quentin.sch...@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.sch...@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 <s...@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.
Subce the macro only returns if there is an error,that could be
important if we are trying to have a descriptive name. Perhaps the
logging bit is less important.
How about ERR_RET() or RET_ON_ERR() ? The last one is getting long too.
> I'm not sure this downside outweighs the benefits to be honest, but I
> also don't write a lot of code these days so /me shrugs
I am thinking of the code here:
https://elixir.bootlin.com/u-boot/v2024.10/source/boot/bootflow_menu.c#L61
Regards,
Simon
