Messages in this thread | | | Date | Fri, 10 Jul 2020 10:10:52 -0300 | From | Bruno Meneguele <> | Subject | Re: [PATCH] doc:kmsg: explictly state the return value in case of SEEK_CUR |
| |
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 [unhandled content-type:application/pgp-signature] | |