lib/errname.c | 1 + lib/vsprintf.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
For code that emits a string representing a usual return value it's
convenient to have a 0 result in a string representation of success
instead of "00000000".
A usecase is tracing where the return value of a callback is emitted,
see
https://lore.kernel.org/linux-pwm/20220916151506.298488-2-u.kleine-koenig@pengutronix.de
for an example.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
lib/errname.c | 1 +
lib/vsprintf.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/errname.c b/lib/errname.c
index 05cbf731545f..2fd430f25059 100644
--- a/lib/errname.c
+++ b/lib/errname.c
@@ -15,6 +15,7 @@
*/
#define E(err) [err + BUILD_BUG_ON_ZERO(err <= 0 || err > 300)] = "-" #err
static const char *names_0[] = {
+ [0] = "SUCCESS",
E(E2BIG),
E(EACCES),
E(EADDRINUSE),
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3c1853a9d1c0..2a27218bab48 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2448,7 +2448,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
return pointer_string(buf, end, ptr, spec);
case 'e':
/* %pe with a non-ERR_PTR gets treated as plain %p */
- if (!IS_ERR(ptr))
+ if (!IS_ERR_OR_NULL(ptr))
return default_pointer(buf, end, ptr, spec);
return err_ptr(buf, end, ptr, spec);
case 'u':
--
2.30.2
On Fri, Sep 30, 2022 at 01:10:50PM +0200, Uwe Kleine-König wrote: > For code that emits a string representing a usual return value it's > convenient to have a 0 result in a string representation of success > instead of "00000000". This is a controversial change. For APIs that comes to my mind it means "OPTIONAL resource NOT FOUND, while no error happened". Doe it mean success? I don't think so. > A usecase is tracing where the return value of a callback is emitted, > see > https://lore.kernel.org/linux-pwm/20220916151506.298488-2-u.kleine-koenig@pengutronix.de > for an example. -- With Best Regards, Andy Shevchenko
On Fri, Sep 30, 2022 at 04:41:24PM +0300, Andy Shevchenko wrote: > On Fri, Sep 30, 2022 at 01:10:50PM +0200, Uwe Kleine-König wrote: > > For code that emits a string representing a usual return value it's > > convenient to have a 0 result in a string representation of success > > instead of "00000000". > > This is a controversial change. For APIs that comes to my mind it means > "OPTIONAL resource NOT FOUND, while no error happened". Doe it mean success? > I don't think so. OK, agreed. Would you feed such a value unchecked to %pe today (i.e. without my patch)? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri 2022-09-30 16:05:31, Uwe Kleine-König wrote: > On Fri, Sep 30, 2022 at 04:41:24PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 30, 2022 at 01:10:50PM +0200, Uwe Kleine-König wrote: > > > For code that emits a string representing a usual return value it's > > > convenient to have a 0 result in a string representation of success > > > instead of "00000000". > > > > This is a controversial change. For APIs that comes to my mind it means > > "OPTIONAL resource NOT FOUND, while no error happened". Doe it mean success? > > I don't think so. > > OK, agreed. Would you feed such a value unchecked to %pe today (i.e. > without my patch)? People are primary interested into debug messages when things does not work as expected. The check might be missing intentionally to show all values or by mistake. The tracepoint, used as motivation for this patch [1], is exactly the situation where return values are printed without any check. The problem is that %pe is used for both pointer and integer return values. They have different semantic. I do not feel comfortable with "improving" one use case and breaking the other. One solution would be to add support for "%de" but this would break things. "%de" is supposed to print the 'e'. For example, it should printk "123e" when the given number is 123. Another solution would be to add modifier for the "%pe" modifier. For example, "%ped". It would mean that the given value is in "int" range. It could even print non-hashed value when it is out of range. [1] https://lore.kernel.org/linux-pwm/20220916151506.298488-2-u.kleine-koenig@pengutronix.de/ Best Regards, Petr
On Fri 2022-09-30 13:10:50, Uwe Kleine-König wrote: > For code that emits a string representing a usual return value it's > convenient to have a 0 result in a string representation of success > instead of "00000000". Does it really always mean success, please? IMHO, if a function returns a pointer then typically only a valid pointer means success. Error code means some reasonable explanation of the failure. And NULL should never happen. For example: struct bla *find_bla(int key) { struct bla *b; /* Try to get bla using the given key */ ... if (succeded) return b; /* Did not find bla for the given key */ return -EINVAL; } It might be used: int process_bla() { struct bla *b; b = get_bla(); if (IS_ERR(b)) return PTR_ERR(b); /* do something with b */ ... } If get_bla() returns NULL then it means a super fault. It means that get_bla() failed and did not know why. IMHO, this patch might do more harm than good. Best Regards, Petr
On Fri, Sep 30, 2022 at 02:14:16PM +0200, Petr Mladek wrote: > On Fri 2022-09-30 13:10:50, Uwe Kleine-König wrote: > > For code that emits a string representing a usual return value it's > > convenient to have a 0 result in a string representation of success > > instead of "00000000". > > Does it really always mean success, please? > > IMHO, if a function returns a pointer then typically only a valid > pointer means success. Error code means some reasonable explanation > of the failure. And NULL should never happen. So your example function doesn't hit the case that we're discussing here because it will never return NULL and so the code path I added isn't used and doesn't make a difference, right? > For example: > > struct bla *find_bla(int key) > { > struct bla *b; > > /* Try to get bla using the given key */ > ... > > if (succeded) > return b; > > /* Did not find bla for the given key */ > return -EINVAL; nitpick: s/-EINVAL/ERR_PTR(-EINVAL)/ > > } > > It might be used: > > int process_bla() > { > struct bla *b; > > b = get_bla(); > if (IS_ERR(b)) > return PTR_ERR(b); > > /* do something with b */ > ... > } > > If get_bla() returns NULL then it means a super fault. It means > that get_bla() failed and did not know why. OK, I think we agree that a function that might return an error pointer shouldn't return NULL with the semantic "This is also an error." Only in combination with such a function you can reasonably object the addition of PTR_ERR(0) meaning "SUCCESS". In such a case the right action is to fix the function. > IMHO, this patch might do more harm than good. Hmm, do you think there are many functions that use both NULL and error pointers to signal a failure? I don't see where the patch might do harm otherwise. In *my* humble opinion it's perfectly fine that a given printk feature results in strange output when it's fed with strange input. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Sep 30, 2022 at 03:24:24PM +0200, Uwe Kleine-König wrote: > On Fri, Sep 30, 2022 at 02:14:16PM +0200, Petr Mladek wrote: > > On Fri 2022-09-30 13:10:50, Uwe Kleine-König wrote: ... > > If get_bla() returns NULL then it means a super fault. It means > > that get_bla() failed and did not know why. > > OK, I think we agree that a function that might return an error pointer > shouldn't return NULL with the semantic "This is also an error." But it neither means "success". -- With Best Regards, Andy Shevchenko
On Fri, Sep 30, 2022 at 04:42:48PM +0300, Andy Shevchenko wrote: > On Fri, Sep 30, 2022 at 03:24:24PM +0200, Uwe Kleine-König wrote: > > On Fri, Sep 30, 2022 at 02:14:16PM +0200, Petr Mladek wrote: > > > On Fri 2022-09-30 13:10:50, Uwe Kleine-König wrote: > > ... > > > > If get_bla() returns NULL then it means a super fault. It means > > > that get_bla() failed and did not know why. > > > > OK, I think we agree that a function that might return an error pointer > > shouldn't return NULL with the semantic "This is also an error." > > But it neither means "success". Have you read and understood the patch and the discussion? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
© 2016 - 2024 Red Hat, Inc.