[PATCH] doc:kmsg: explictly state the return value in case of SEEK_CUR

[PATCH] doc:kmsg: explictly state the return value in case of SEEK_CUR

[Bruno Meneguele]

The commit 625d3449788f ("Revert "kernel/printk: add kmsg SEEK_CUR
handling"") reverted a change done to the return value in case a SEEK_CUR
operation was performed for kmsg buffer based on the fact that different
userspace apps were handling the new return value (-ESPIPE) in different
ways, breaking them.

At the same time -ESPIPE was the wrong decision because kmsg /does support/
seek() but doesn't follow the "normal" behavior userspace is used to.
Because of that and also considering the time -EINVAL has been used, it was
decided to keep this way to avoid more userspace breakage.

This patch adds an official statement to the kmsg documentation pointing to
the current return value for SEEK_CUR, -EINVAL, thus userspace libraries and
apps can refer to it for a definitive guide on what to expected.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
 Documentation/ABI/testing/dev-kmsg | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/ABI/testing/dev-kmsg b/Documentation/ABI/testing/dev-kmsg
index f307506eb54c..9bc854faaa79 100644
--- a/Documentation/ABI/testing/dev-kmsg
+++ b/Documentation/ABI/testing/dev-kmsg
@@ -56,6 +56,11 @@ Description:	The /dev/kmsg character device node provides userspace access
 		  seek after the last record available at the time
 		  the last SYSLOG_ACTION_CLEAR was issued.
 
+		Considering that the seek operation is supported but has 
+		special meaning to the device, any attempt to seek specific
+		positions on the buffer (i.e.  using SEEK_CUR) results in an
+		-EINVAL error returned to userspace.
+		
 		The output format consists of a prefix carrying the syslog
 		prefix including priority and facility, the 64 bit message
 		sequence number and the monotonic timestamp in microseconds,
-- 
2.26.2

Re: [PATCH] doc:kmsg: explictly state the return value in case of SEEK_CUR

[Petr Mladek]

On Thu 2020-07-09 12:54:15, Bruno Meneguele wrote:
> The commit 625d3449788f ("Revert "kernel/printk: add kmsg SEEK_CUR
> handling"") reverted a change done to the return value in case a SEEK_CUR
> operation was performed for kmsg buffer based on the fact that different
> userspace apps were handling the new return value (-ESPIPE) in different
> ways, breaking them.
> 
> At the same time -ESPIPE was the wrong decision because kmsg /does support/
> seek() but doesn't follow the "normal" behavior userspace is used to.
> Because of that and also considering the time -EINVAL has been used, it was
> decided to keep this way to avoid more userspace breakage.
> 
> This patch adds an official statement to the kmsg documentation pointing to
> the current return value for SEEK_CUR, -EINVAL, thus userspace libraries and
> apps can refer to it for a definitive guide on what to expected.
> 
> --- a/Documentation/ABI/testing/dev-kmsg
> +++ b/Documentation/ABI/testing/dev-kmsg
> @@ -56,6 +56,11 @@ Description:	The /dev/kmsg character device node provides userspace access
>  		  seek after the last record available at the time
>  		  the last SYSLOG_ACTION_CLEAR was issued.
>  
> +		Considering that the seek operation is supported but has 
> +		special meaning to the device, any attempt to seek specific
> +		positions on the buffer (i.e.  using SEEK_CUR) results in an
> +		-EINVAL error returned to userspace.

Sigh, I see that devkmsg_llseek() returns -ESPIPE when offset is not
zero. This is a real mess.

I wonder if we could afford to switch this one to -EINVAL and reduce
the mess.

Anyway, for a random reader, it might be pretty unclear what is
exactly special about /dev/kmsg. I wonder if the following might
be more explanatory and strightforward:

		Other seek operations or offsets are not supported because of
		the special behavior. The device allows to read or write
		only whole variable lenght messages that are stored in
		a ring buffer.

		Because of the non-standard behavior also the error values
		are non-standardand. -ESPIPE is returned for non-zero offset.
		-EINVAL is returned for other operations, e.g. SEEK_CUR.
		This behavior is historical and could not be modified
		wihtout the risk of breaking userspace.


Finally, only few people read documentation. We should add some
warning also to the code. I think about a something like:

/*
 * Be careful when modifying this function!!!
 *
 * Only few operations are supported because the device works only with
 * the entire variable length messages. Non-standard error values are
 * returned in the other cases. User space applications might depend
 * on this behavior.
 */

Best Regards,
Petr

Re: [PATCH] doc:kmsg: explictly state the return value in case of SEEK_CUR

[Bruno Meneguele]

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

On Fri, Jul 10, 2020 at 02:19:23PM +0200, Petr Mladek wrote:
> On Thu 2020-07-09 12:54:15, Bruno Meneguele wrote:
> > The commit 625d3449788f ("Revert "kernel/printk: add kmsg SEEK_CUR
> > handling"") reverted a change done to the return value in case a SEEK_CUR
> > operation was performed for kmsg buffer based on the fact that different
> > userspace apps were handling the new return value (-ESPIPE) in different
> > ways, breaking them.
> > 
> > At the same time -ESPIPE was the wrong decision because kmsg /does support/
> > seek() but doesn't follow the "normal" behavior userspace is used to.
> > Because of that and also considering the time -EINVAL has been used, it was
> > decided to keep this way to avoid more userspace breakage.
> > 
> > This patch adds an official statement to the kmsg documentation pointing to
> > the current return value for SEEK_CUR, -EINVAL, thus userspace libraries and
> > apps can refer to it for a definitive guide on what to expected.
> > 
> > --- a/Documentation/ABI/testing/dev-kmsg
> > +++ b/Documentation/ABI/testing/dev-kmsg
> > @@ -56,6 +56,11 @@ Description:	The /dev/kmsg character device node provides userspace access
> >  		  seek after the last record available at the time
> >  		  the last SYSLOG_ACTION_CLEAR was issued.
> >  
> > +		Considering that the seek operation is supported but has 
> > +		special meaning to the device, any attempt to seek specific
> > +		positions on the buffer (i.e.  using SEEK_CUR) results in an
> > +		-EINVAL error returned to userspace.
> 
> Sigh, I see that devkmsg_llseek() returns -ESPIPE when offset is not
> zero. This is a real mess.
> 
> I wonder if we could afford to switch this one to -EINVAL and reduce
> the mess.
> 

That's a really good question for which I think the answer is something
close to: "that's impossible to predict". I could try some in-house (at
redhat) automated tests to see if something break, but I doubt it is
enough to catch any breakage.

> Anyway, for a random reader, it might be pretty unclear what is
> exactly special about /dev/kmsg. I wonder if the following might
> be more explanatory and strightforward:
> 
> 		Other seek operations or offsets are not supported because of
> 		the special behavior. The device allows to read or write
> 		only whole variable lenght messages that are stored in
> 		a ring buffer.
> 
> 		Because of the non-standard behavior also the error values
> 		are non-standardand. -ESPIPE is returned for non-zero offset.
> 		-EINVAL is returned for other operations, e.g. SEEK_CUR.
> 		This behavior is historical and could not be modified
> 		wihtout the risk of breaking userspace.
> 

Yes, no doubt it's better!

> 
> Finally, only few people read documentation. We should add some
> warning also to the code. I think about a something like:
> 
> /*
>  * Be careful when modifying this function!!!
>  *
>  * Only few operations are supported because the device works only with
>  * the entire variable length messages. Non-standard error values are
>  * returned in the other cases. User space applications might depend
>  * on this behavior.
>  */
> 

Agreed.

I'm going to prepare a v2 of the patch adding the comments.

Thanks Petr!

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

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