| Message ID | 20221128132106.291101-1-beanhuo@iokpp.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libtracefs: Fix wrong return value in tracefs_tracing_dir_is_mounted() | expand |
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; > } > > /**
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
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
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; > > > ... > > > }
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; }
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
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; } /**