Hi Simon,
On 4/3/25 7:57 PM, Simon Glass wrote:
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?
I don't think the balance between brevity and potential confusion is
appropriate here, I would prefer not to have this.
Cheers,
Quentin
