All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe
@ 2022-09-30 11:10 Uwe Kleine-König
  2022-09-30 12:14 ` Petr Mladek
  2022-09-30 13:41 ` Andy Shevchenko
  0 siblings, 2 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2022-09-30 11:10 UTC (permalink / raw)
  To: Petr Mladek, Steven Rostedt, Sergey Senozhatsky
  Cc: Andy Shevchenko, Rasmus Villemoes, linux-kernel, kernel

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe
  2022-09-30 11:10 [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe Uwe Kleine-König
@ 2022-09-30 12:14 ` Petr Mladek
  2022-09-30 13:24   ` Uwe Kleine-König
  2022-09-30 13:41 ` Andy Shevchenko
  1 sibling, 1 reply; 8+ messages in thread
From: Petr Mladek @ 2022-09-30 12:14 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Steven Rostedt, Sergey Senozhatsky, Andy Shevchenko,
	Rasmus Villemoes, linux-kernel, kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe
  2022-09-30 12:14 ` Petr Mladek
@ 2022-09-30 13:24   ` Uwe Kleine-König
  2022-09-30 13:42     ` Andy Shevchenko
  0 siblings, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2022-09-30 13:24 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Rasmus Villemoes, linux-kernel, Steven Rostedt,
	Sergey Senozhatsky, kernel, Andy Shevchenko

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

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/ |

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe
  2022-09-30 11:10 [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe Uwe Kleine-König
  2022-09-30 12:14 ` Petr Mladek
@ 2022-09-30 13:41 ` Andy Shevchenko
  2022-09-30 14:05   ` Uwe Kleine-König
  1 sibling, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2022-09-30 13:41 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Petr Mladek, Steven Rostedt, Sergey Senozhatsky,
	Rasmus Villemoes, linux-kernel, kernel

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe
  2022-09-30 13:24   ` Uwe Kleine-König
@ 2022-09-30 13:42     ` Andy Shevchenko
  2022-09-30 13:48       ` Uwe Kleine-König
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2022-09-30 13:42 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Petr Mladek, Rasmus Villemoes, linux-kernel, Steven Rostedt,
	Sergey Senozhatsky, kernel

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe
  2022-09-30 13:42     ` Andy Shevchenko
@ 2022-09-30 13:48       ` Uwe Kleine-König
  0 siblings, 0 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2022-09-30 13:48 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Petr Mladek, Rasmus Villemoes, linux-kernel, Steven Rostedt,
	Sergey Senozhatsky, kernel

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

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/ |

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe
  2022-09-30 13:41 ` Andy Shevchenko
@ 2022-09-30 14:05   ` Uwe Kleine-König
  2022-10-03 13:26     ` Petr Mladek
  0 siblings, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2022-09-30 14:05 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Petr Mladek, Rasmus Villemoes, linux-kernel, Steven Rostedt,
	Sergey Senozhatsky, kernel

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

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/ |

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe
  2022-09-30 14:05   ` Uwe Kleine-König
@ 2022-10-03 13:26     ` Petr Mladek
  0 siblings, 0 replies; 8+ messages in thread
From: Petr Mladek @ 2022-10-03 13:26 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Andy Shevchenko, Rasmus Villemoes, linux-kernel, Steven Rostedt,
	Sergey Senozhatsky, kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-10-03 13:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-30 11:10 [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe Uwe Kleine-König
2022-09-30 12:14 ` Petr Mladek
2022-09-30 13:24   ` Uwe Kleine-König
2022-09-30 13:42     ` Andy Shevchenko
2022-09-30 13:48       ` Uwe Kleine-König
2022-09-30 13:41 ` Andy Shevchenko
2022-09-30 14:05   ` Uwe Kleine-König
2022-10-03 13:26     ` Petr Mladek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.