Hi Tom,
On Wed, 2 Apr 2025 at 06:19, Tom Rini <tr...@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 <tr...@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 <tr...@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.sch...@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.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.
> > > > > > > >
> > > > > > >
> > > > > > > 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?
Regards,
Simon
