[PATCH] trace: Return ENOTCONN instead of EBADF

[PATCH] trace: Return ENOTCONN instead of EBADF

[Peter Enderborg]

When there is no clients listening on event the trace return
EBADF. The file is not a bad file descriptor and to get the
userspace able to do a proper error handling it need a different
error code that separate a bad file descriptor from a empty listening.

Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
---
 kernel/trace/trace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d3e5de717df2..6e592bf736df 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -6651,8 +6651,8 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
 	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
 					    irq_flags, preempt_count());
 	if (unlikely(!event))
-		/* Ring buffer disabled, return as if not open for write */
-		return -EBADF;
+		/* Ring buffer disabled, return as if not connected */
+		return -ENOTCONN;
 
 	entry = ring_buffer_event_data(event);
 	entry->ip = _THIS_IP_;
@@ -6731,8 +6731,8 @@ tracing_mark_raw_write(struct file *filp, const char __user *ubuf,
 	event = __trace_buffer_lock_reserve(buffer, TRACE_RAW_DATA, size,
 					    irq_flags, preempt_count());
 	if (!event)
-		/* Ring buffer disabled, return as if not open for write */
-		return -EBADF;
+		/* Ring buffer disabled, return not connected */
+		return -ENOTCONN;
 
 	entry = ring_buffer_event_data(event);
 
-- 
2.17.1

Re: [PATCH] trace: Return ENOTCONN instead of EBADF

[Steven Rostedt]

On Mon, 12 Oct 2020 10:26:42 +0200
Peter Enderborg <peter.enderborg@sony.com> wrote:

> When there is no clients listening on event the trace return
> EBADF. The file is not a bad file descriptor and to get the
> userspace able to do a proper error handling it need a different
> error code that separate a bad file descriptor from a empty listening.

I have no problem with this patch, but your description is incorrect. And
before making this change, I want to make sure that what you think is
happening is actually happening.

This has nothing to do with "clients listening". This happens when the ring
buffer is disabled for some reason. The most likely case of this happening
is if someone sets /sys/kernel/tracing/tracing_on to zero.

If this is still something you want applied, please update the change log
to a more accurate scenario.

Thanks,

-- Steve


> 
> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
> ---
>  kernel/trace/trace.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d3e5de717df2..6e592bf736df 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6651,8 +6651,8 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>  	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
>  					    irq_flags, preempt_count());
>  	if (unlikely(!event))
> -		/* Ring buffer disabled, return as if not open for write */
> -		return -EBADF;
> +		/* Ring buffer disabled, return as if not connected */
> +		return -ENOTCONN;
>  
>  	entry = ring_buffer_event_data(event);
>  	entry->ip = _THIS_IP_;
> @@ -6731,8 +6731,8 @@ tracing_mark_raw_write(struct file *filp, const char __user *ubuf,
>  	event = __trace_buffer_lock_reserve(buffer, TRACE_RAW_DATA, size,
>  					    irq_flags, preempt_count());
>  	if (!event)
> -		/* Ring buffer disabled, return as if not open for write */
> -		return -EBADF;
> +		/* Ring buffer disabled, return not connected */
> +		return -ENOTCONN;
>  
>  	entry = ring_buffer_event_data(event);
>  

Re: [PATCH] trace: Return ENOTCONN instead of EBADF

[Enderborg, Peter]

On 10/12/20 3:53 PM, Steven Rostedt wrote:
> On Mon, 12 Oct 2020 10:26:42 +0200
> Peter Enderborg <peter.enderborg@sony.com> wrote:
>
>> When there is no clients listening on event the trace return
>> EBADF. The file is not a bad file descriptor and to get the
>> userspace able to do a proper error handling it need a different
>> error code that separate a bad file descriptor from a empty listening.
> I have no problem with this patch, but your description is incorrect. And
> before making this change, I want to make sure that what you think is
> happening is actually happening.
>
> This has nothing to do with "clients listening". This happens when the ring
> buffer is disabled for some reason. The most likely case of this happening
> is if someone sets /sys/kernel/tracing/tracing_on to zero.

I see that as no one is listening. You start to listen by setting this tracing on
some instance, but that is for trace_pipe. Is it the same flag for raw access and all ways you
can invoke a trace?

Would

"When there is no instances listening on events the trace return
EBADF, it is when the tracing_on is off globally. The file is not a bad file
descriptor and to get the userspace able to do a proper error handling it need a different
error code that separate a bad file descriptor from a empty listening."

be a ok?


> If this is still something you want applied, please update the change log
> to a more accurate scenario.
>
> Thanks,
>
> -- Steve
>
>
>> Signed-off-by: Peter Enderborg <peter.enderborg@sony.com>
>> ---
>>  kernel/trace/trace.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index d3e5de717df2..6e592bf736df 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -6651,8 +6651,8 @@ tracing_mark_write(struct file *filp, const char __user *ubuf,
>>  	event = __trace_buffer_lock_reserve(buffer, TRACE_PRINT, size,
>>  					    irq_flags, preempt_count());
>>  	if (unlikely(!event))
>> -		/* Ring buffer disabled, return as if not open for write */
>> -		return -EBADF;
>> +		/* Ring buffer disabled, return as if not connected */
>> +		return -ENOTCONN;
>>  
>>  	entry = ring_buffer_event_data(event);
>>  	entry->ip = _THIS_IP_;
>> @@ -6731,8 +6731,8 @@ tracing_mark_raw_write(struct file *filp, const char __user *ubuf,
>>  	event = __trace_buffer_lock_reserve(buffer, TRACE_RAW_DATA, size,
>>  					    irq_flags, preempt_count());
>>  	if (!event)
>> -		/* Ring buffer disabled, return as if not open for write */
>> -		return -EBADF;
>> +		/* Ring buffer disabled, return not connected */
>> +		return -ENOTCONN;
>>  
>>  	entry = ring_buffer_event_data(event);
>>  

Re: [PATCH] trace: Return ENOTCONN instead of EBADF

[Steven Rostedt]

On Mon, 12 Oct 2020 14:26:48 +0000
"Enderborg, Peter" <Peter.Enderborg@sony.com> wrote:

> On 10/12/20 3:53 PM, Steven Rostedt wrote:
> > On Mon, 12 Oct 2020 10:26:42 +0200
> > Peter Enderborg <peter.enderborg@sony.com> wrote:
> >  
> >> When there is no clients listening on event the trace return
> >> EBADF. The file is not a bad file descriptor and to get the
> >> userspace able to do a proper error handling it need a different
> >> error code that separate a bad file descriptor from a empty listening.  
> > I have no problem with this patch, but your description is incorrect. And
> > before making this change, I want to make sure that what you think is
> > happening is actually happening.
> >
> > This has nothing to do with "clients listening". This happens when the ring
> > buffer is disabled for some reason. The most likely case of this happening
> > is if someone sets /sys/kernel/tracing/tracing_on to zero.  
> 
> I see that as no one is listening. You start to listen by setting this tracing on
> some instance, but that is for trace_pipe. Is it the same flag for raw access and all ways you
> can invoke a trace?

I don't know what you mean by "setting this tracing on some instance".

When you boot up, who is listening? tracing_on is enabled. But there's no
listener.


> 
> Would
> 
> "When there is no instances listening on events the trace return

What instance are you talking about? What do you think needs to be
listening. And no, trace_pipe plays zero role in this.

> EBADF, it is when the tracing_on is off globally. The file is not a bad file
> descriptor and to get the userspace able to do a proper error handling it need a different
> error code that separate a bad file descriptor from a empty listening."
> 
> be a ok?
> 

No. Because I don't understand any of the above!

There's no a producer / consumer here. It's a read / write enabled or
disabled. If the ring buffer is disabled from write, and you try to write
to the ring buffer, you get an error.

Basically, it's the same analogy as trying to write to a read-only file.
Perhaps the proper error is EPERM.

-- Steve