[PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[Uwe Kleine-König]

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

Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[Petr Mladek]

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

Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[Uwe Kleine-König]

[-- 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 --]

Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[Andy Shevchenko]

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

Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[Uwe Kleine-König]

[-- 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 --]

Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[Andy Shevchenko]

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

Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[Uwe Kleine-König]

[-- 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 --]

Re: [PATCH] printf: Emit "SUCCESS" if NULL is passed for %pe

[Petr Mladek]

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