[PATCH] libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted()

[PATCH] libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

If it eventually mounts successfully, it should return 1 instead of
0, otherwise it will make the caller's verification logic more complicated

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 src/tracefs-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/tracefs-utils.c b/src/tracefs-utils.c
index 777912e..5d6e977 100644
--- a/src/tracefs-utils.c
+++ b/src/tracefs-utils.c
@@ -188,7 +188,7 @@ int tracefs_tracing_dir_is_mounted(bool mount, const char **path)
 		return -1;
 	if (path)
 		*path = dir;
-	return 0;
+	return 1;
 }
 
 /**
-- 
2.25.1

Re: [PATCH] libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted()

[Steven Rostedt]

On Mon, 28 Nov 2022 14:21:06 +0100
Bean Huo <beanhuo@iokpp.de> wrote:

> From: Bean Huo <beanhuo@micron.com>
> 
> If it eventually mounts successfully, it should return 1 instead of
> 0, otherwise it will make the caller's verification logic more complicated

The man page shows:

  The tracefs_tracing_dir_is_mounted() returns 1 if the tracing directory is
  already mounted, 0 if it is not, and -1 on error.

If you only want to mount it and not care if it was already mounted then
use tracefs_tracing_dir(), as it will return the path of the mount point
and try to mount it if it is not already, or NULL if it could not mount it.

This function is specifically created to tell the application if it mounted
or not, so that it could unmount it when it is done. We have applications
that do this.

So, no, I'm not taking this change. It breaks the use case for this
function.

-- Steve


> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  src/tracefs-utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/tracefs-utils.c b/src/tracefs-utils.c
> index 777912e..5d6e977 100644
> --- a/src/tracefs-utils.c
> +++ b/src/tracefs-utils.c
> @@ -188,7 +188,7 @@ int tracefs_tracing_dir_is_mounted(bool mount, const char **path)
>  		return -1;
>  	if (path)
>  		*path = dir;
> -	return 0;
> +	return 1;
>  }
>  
>  /**

Re: [PATCH] libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted()

[Bean Huo]

On Mon, 2022-11-28 at 10:24 -0500, Steven Rostedt wrote:
> On Mon, 28 Nov 2022 14:21:06 +0100
> 
> Bean Huo <beanhuo@iokpp.de> wrote:
> 
> 
> 
> > From: Bean Huo <beanhuo@micron.com>
> > If it eventually mounts successfully, it should return 1 instead of
> > 0, otherwise it will make the caller's verification logic more
> > complicated
> 
> 
> The man page shows:
> 
> 
> 
>   The tracefs_tracing_dir_is_mounted() returns 1 if the tracing
> directory is
> 
>   already mounted, 0 if it is not, and -1 on error.
> 
> 
> 
> If you only want to mount it and not care if it was already mounted
> then
> 
> use tracefs_tracing_dir(), as it will return the path of the mount
> point
> 
> and try to mount it if it is not already, or NULL if it could not
> mount it.
> 
> 
> 

Hi Steve,

I used to reply on tracefs_tracing_dir(). But after updated the
libtracefs yesterday, I found my app doesn't work as before, then
checked the changelog, and switch to use
tracefs_tracing_dir_is_mounted. 

in your latest libtracefs:


const char *tracefs_tracing_dir(void) {

...
tracing_dir = trace_find_tracing_dir(false);//shold be changed to true?
...
	
}


__hidden char *trace_find_tracing_dir(bool debugfs)
{
        return find_tracing_dir(debugfs, false);
}



static char *find_tracing_dir(bool debugfs, bool mount) {

...
	if (!mount || mount_debugfs() < 0)
		return NULL;


...


}


Kind regards,
Bean

Re: [PATCH] libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted()

[Bean Huo]

On Mon, 2022-11-28 at 16:41 +0100, Bean Huo wrote:
> On Mon, 2022-11-28 at 10:24 -0500, Steven Rostedt wrote:
> > On Mon, 28 Nov 2022 14:21:06 +0100
> > 
> > Bean Huo <beanhuo@iokpp.de> wrote:
> > 
> > 
> > 
> > > From: Bean Huo <beanhuo@micron.com>
> > > If it eventually mounts successfully, it should return 1 instead
> > > of
> > > 0, otherwise it will make the caller's verification logic more
> > > complicated
> > 
> > The man page shows:
> > 
> > 
> > 
> >   The tracefs_tracing_dir_is_mounted() returns 1 if the tracing
> > directory is
> > 
> >   already mounted, 0 if it is not, and -1 on error.
> > 
> > 
> > 
> > If you only want to mount it and not care if it was already mounted
> > then
> > 
> > use tracefs_tracing_dir(), as it will return the path of the mount
> > point
> > 
> > and try to mount it if it is not already, or NULL if it could not
> > mount it.
> > 
> > 
> > 
> 
> Hi Steve,
> 
> I used to reply on tracefs_tracing_dir(). But after updated the
> libtracefs yesterday, I found my app doesn't work as before, then
> checked the changelog, and switch to use
> tracefs_tracing_dir_is_mounted. 
> 
> in your latest libtracefs:
> 
> 
> const char *tracefs_tracing_dir(void) {
> 
> ...
> tracing_dir = trace_find_tracing_dir(false);//shold be changed to
> true?
> ...
> 	
> }
> 
> 
> __hidden char *trace_find_tracing_dir(bool debugfs)
> {
>         return find_tracing_dir(debugfs, false);
> }
> 
> 
> 
> static char *find_tracing_dir(bool debugfs, bool mount) {
> 
> ...
> 	if (!mount || mount_debugfs() < 0)
> 		return NULL;
> 
> 
> ...
> 
> 
> }
> 
> 


instead of changing tracefs_tracing_dir_is_mounted(), or we could
change trace_find_tracing_dir(), let it mount the tracefs in case of it
is not mounted when we call tracefs_tracing_dir():



--- a/src/tracefs-utils.c
+++ b/src/tracefs-utils.c
@@ -200,7 +200,7 @@ int tracefs_tracing_dir_is_mounted(bool mount,
const char **path)
  */
 __hidden char *trace_find_tracing_dir(bool debugfs)
 {
-       return find_tracing_dir(debugfs, false);
+       return find_tracing_dir(debugfs, true);
 }


Kind regard,
Bean

Re: [PATCH] libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted()

[Steven Rostedt]

On Mon, 28 Nov 2022 16:41:54 +0100
Bean Huo <beanhuo@iokpp.de> wrote:

> I used to reply on tracefs_tracing_dir(). But after updated the
> libtracefs yesterday, I found my app doesn't work as before, then
> checked the changelog, and switch to use
> tracefs_tracing_dir_is_mounted. 

OK, so this is a regression and needs to be fixed.


> 
> in your latest libtracefs:
> 
> 
> const char *tracefs_tracing_dir(void) {
> 
> ...
> tracing_dir = trace_find_tracing_dir(false);//shold be changed to true?
> ...

Ah, I think I see the issue here. If it returns NULL, then we need to do
more. I'll take a look into this later today.

Thanks for reporting this.

-- Steve


> 	
> }
> 
> 
> __hidden char *trace_find_tracing_dir(bool debugfs)
> {
>         return find_tracing_dir(debugfs, false);
> }
> 
> 
> 
> static char *find_tracing_dir(bool debugfs, bool mount) {
> 
> ...
> 	if (!mount || mount_debugfs() < 0)
> 		return NULL;
> 
> 
> ...
> 
> 
> }

Re: [PATCH] libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted()

[Steven Rostedt]

On Mon, 28 Nov 2022 13:37:15 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> Ah, I think I see the issue here. If it returns NULL, then we need to do
> more. I'll take a look into this later today.

Can you test this patch?

Thanks,

-- Steve

diff --git a/src/tracefs-utils.c b/src/tracefs-utils.c
index 777912e46821..d91ff40eee87 100644
--- a/src/tracefs-utils.c
+++ b/src/tracefs-utils.c
@@ -245,7 +245,7 @@ const char *tracefs_tracing_dir(void)
 	if (tracing_dir)
 		return tracing_dir;
 
-	tracing_dir = trace_find_tracing_dir(false);
+	tracing_dir = find_tracing_dir(false, true);
 	return tracing_dir;
 }
 
@@ -263,7 +263,7 @@ const char *tracefs_debug_dir(void)
 	if (debug_dir)
 		return debug_dir;
 
-	debug_dir = trace_find_tracing_dir(true);
+	debug_dir = find_tracing_dir(true, true);
 	return debug_dir;
 }
 

Re: [PATCH] libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted()

[Bean Huo]

On Tue, 2022-12-06 at 16:13 -0500, Steven Rostedt wrote:
> On Mon, 28 Nov 2022 13:37:15 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Ah, I think I see the issue here. If it returns NULL, then we need
> > to do
> > more. I'll take a look into this later today.
> 
> Can you test this patch?
> 
> Thanks,
> 
> -- Steve
> 
> diff --git a/src/tracefs-utils.c b/src/tracefs-utils.c
> index 777912e46821..d91ff40eee87 100644
> --- a/src/tracefs-utils.c
> +++ b/src/tracefs-utils.c
> @@ -245,7 +245,7 @@ const char *tracefs_tracing_dir(void)
>  	if (tracing_dir)
>  		return tracing_dir;
>  
> -	tracing_dir = trace_find_tracing_dir(false);
> +	tracing_dir = find_tracing_dir(false, true);
>  	return tracing_dir;
>  }
>  
> @@ -263,7 +263,7 @@ const char *tracefs_debug_dir(void)
>  	if (debug_dir)
>  		return debug_dir;
>  
> -	debug_dir = trace_find_tracing_dir(true);
> +	debug_dir = find_tracing_dir(true, true);
>  	return debug_dir;
>  }
>  


yes, it also works, thanks!

Kind regards,
Bean