[ndctl PATCH] daxctl: Replace basename() usage with strrchr()

[ndctl PATCH] daxctl: Replace basename() usage with strrchr()

[Alison Schofield]

A user reports that ndctl fails to compile on MUSL systems:

daxctl/device.c: In function 'parse_device_options':
daxctl/device.c:377:26: error: implicit declaration of function 'basename' [-Wimplicit-function-declaration]
  377 |                 device = basename(argv[0]);
      |                          ^~~~~~~~

There are two versions of basename() with different behaviors:
	GNU basename() from <string.h>: doesn't modify its argument
	POSIX basename() from <libgen.h>: may modify its argument
glibc provides both versions, while MUSL libc only provides the POSIX
version.

In daxctl/device.c, basename() is called without any header, relying
on the GNU extension being implicitly available. And in daxctl/lib/
libdaxctl.c, libgen.h is included and the POSIX basename() used,
which works but is needlessly complex as POSIX basename() can modify
its argument.

Rather than conditionally including headers or dealing with platform
differences, replace both basename() usages with a new implementation
using strrchar to find the last '/' in the path and return everything
after it, or the whole string if no '/' is found.

Closes: https://github.com/pmem/ndctl/issues/283
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 daxctl/device.c        | 8 +++++---
 daxctl/lib/libdaxctl.c | 4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/daxctl/device.c b/daxctl/device.c
index e3993b17c260..44a0a0ddb1d4 100644
--- a/daxctl/device.c
+++ b/daxctl/device.c
@@ -362,11 +362,13 @@ static const char *parse_device_options(int argc, const char **argv,
 	};
 	unsigned long long units = 1;
 	int i, rc = 0;
-	char *device = NULL;
+	const char *device = NULL;
 
 	argc = parse_options(argc, argv, options, u, 0);
-	if (argc > 0)
-		device = basename(argv[0]);
+	if (argc > 0) {
+		device = strrchr(argv[0], '/');
+		device = device ? device + 1 : argv[0];
+	}
 
 	/* Handle action-agnostic non-option arguments */
 	if (argc == 0 &&
diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index b7fa0de0b73d..eee1d02c3714 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -3,7 +3,6 @@
 #include <stdio.h>
 #include <errno.h>
 #include <limits.h>
-#include <libgen.h>
 #include <stdlib.h>
 #include <dirent.h>
 #include <unistd.h>
@@ -408,7 +407,8 @@ DAXCTL_EXPORT int daxctl_dev_is_system_ram_capable(struct daxctl_dev *dev)
 	if (!mod_path)
 		return false;
 
-	mod_base = basename(mod_path);
+	mod_base = strrchr(mod_path, '/');
+	mod_base = mod_base ? mod_base + 1 : mod_path;
 	if (strcmp(mod_base, dax_modules[DAXCTL_DEV_MODE_RAM]) == 0) {
 		free(mod_path);
 		return true;
-- 
2.37.3

Re: [ndctl PATCH] daxctl: Replace basename() usage with strrchr()

[Marc Herbert]

Hi Alison,

Alison Schofield <alison.schofield@intel.com> writes:
>  	argc = parse_options(argc, argv, options, u, 0);
> -	if (argc > 0)
> -		device = basename(argv[0]);
> +	if (argc > 0) {
> +		device = strrchr(argv[0], '/');
> +		device = device ? device + 1 : argv[0];
> +	}
>  


1. I would add a one-line comment in both places, something like "This
is like basename but without the bugs and portability issues" because:

  1.a) It's much faster to read such a comment than understanding the code.
  1.b) Not everyone knows how much of GNU/POSIX disaster is "basename".
       You summarized it well in the commit message but it's unlikely
       anyone will fetch the commit message from git without such a comment.

  To avoid duplicating the comment, a small "my_basename()" inline
  function would not hurt while at it.


2. I believe this (unlike basename) returns an empty string when the
   argument has a trailing slash. Now, an argument with a trailing slash
   would probably be garbage and I'm OK with the "Garbage IN, garbage
   OUT" principle. BUT I also believe in the "Proportionate Response"
   principle, which means a small amount of garbage IN should IMHO not
   be punished by some utterly baffling error message or (much worse) a
   crash. Did you/could you test what happens with a trailing slash? If
   the resulting failure makes some kind of sense then don't change
   anything.

Note even when rare in interactive use, "Garbage IN" becomes more
frequent in automation. Then good luck making sense of a cryptic error
when you can't even reproduce the elaborate test configuration and test
bugs that trigger it.

Re: [ndctl PATCH] daxctl: Replace basename() usage with strrchr()

[Alison Schofield]

On Thu, Jan 15, 2026 at 03:54:54PM -0800, Marc Herbert wrote:
> Hi Alison,
> 
> Alison Schofield <alison.schofield@intel.com> writes:
> >  	argc = parse_options(argc, argv, options, u, 0);
> > -	if (argc > 0)
> > -		device = basename(argv[0]);
> > +	if (argc > 0) {
> > +		device = strrchr(argv[0], '/');
> > +		device = device ? device + 1 : argv[0];
> > +	}
> >  
> 
> 
> 1. I would add a one-line comment in both places, something like "This
> is like basename but without the bugs and portability issues" because:
> 
>   1.a) It's much faster to read such a comment than understanding the code.
>   1.b) Not everyone knows how much of GNU/POSIX disaster is "basename".
>        You summarized it well in the commit message but it's unlikely
>        anyone will fetch the commit message from git without such a comment.
> 
>   To avoid duplicating the comment, a small "my_basename()" inline
>   function would not hurt while at it.

Thanks for the review.

I'm headed down the Dan suggested path of adding a helper.
> 
> 
> 2. I believe this (unlike basename) returns an empty string when the
>    argument has a trailing slash. Now, an argument with a trailing slash
>    would probably be garbage and I'm OK with the "Garbage IN, garbage
>    OUT" principle. BUT I also believe in the "Proportionate Response"
>    principle, which means a small amount of garbage IN should IMHO not
>    be punished by some utterly baffling error message or (much worse) a
>    crash. Did you/could you test what happens with a trailing slash? If
>    the resulting failure makes some kind of sense then don't change
>    anything.

With the implementation moved to a new helper, path_basename()
I've tested these: 

assert(strcmp(path_basename("/usr/bin/foo"), "foo") == 0);
assert(strcmp(path_basename("foo"), "foo") == 0);
assert(strcmp(path_basename("/usr/bin/foo/"), "") == 0);
assert(strcmp(path_basename("/"), "") == 0);
assert(strcmp(path_basename(""), "") == 0);

I think this sticks w gigo principle and 'proportionate response' too.
It's safe for valid paths, safe for unusual but harmless paths, no
crashes or undefined behavior, and the thing that inspired this, is
that it behave the same across libc implementations.

Watch for the v2 and let me know if i missed any string tests above.

> 
> Note even when rare in interactive use, "Garbage IN" becomes more
> frequent in automation. Then good luck making sense of a cryptic error
> when you can't even reproduce the elaborate test configuration and test
> bugs that trigger it.
> 

Re: [ndctl PATCH] daxctl: Replace basename() usage with strrchr()

[Alison Schofield]

On Thu, Jan 15, 2026 at 07:17:23PM -0800, Alison Schofield wrote:
> On Thu, Jan 15, 2026 at 03:54:54PM -0800, Marc Herbert wrote:
> > Hi Alison,
> > 
> > Alison Schofield <alison.schofield@intel.com> writes:
> > >  	argc = parse_options(argc, argv, options, u, 0);
> > > -	if (argc > 0)
> > > -		device = basename(argv[0]);
> > > +	if (argc > 0) {
> > > +		device = strrchr(argv[0], '/');
> > > +		device = device ? device + 1 : argv[0];
> > > +	}
> > >  
> > 
> > 
> > 1. I would add a one-line comment in both places, something like "This
> > is like basename but without the bugs and portability issues" because:
> > 
> >   1.a) It's much faster to read such a comment than understanding the code.
> >   1.b) Not everyone knows how much of GNU/POSIX disaster is "basename".
> >        You summarized it well in the commit message but it's unlikely
> >        anyone will fetch the commit message from git without such a comment.
> > 
> >   To avoid duplicating the comment, a small "my_basename()" inline
> >   function would not hurt while at it.
> 
> Thanks for the review.
> 
> I'm headed down the Dan suggested path of adding a helper.
  meant to say - Marc and Dan suggested path :)

Re: [ndctl PATCH] daxctl: Replace basename() usage with strrchr()

[dan.j.williams]

Alison Schofield wrote:
[..]
> Rather than conditionally including headers or dealing with platform
> differences, replace both basename() usages with a new implementation
> using strrchar to find the last '/' in the path and return everything
> after it, or the whole string if no '/' is found.

This feels like it wants a helper rather than a new open-coded thing.
For example, devpath_to_devname() is an existing similar helper.

Alternatively, just create a local helper called basename when the
C-library is missing the GNU version.

Re: [ndctl PATCH] daxctl: Replace basename() usage with strrchr()

[Alison Schofield]

On Thu, Jan 15, 2026 at 06:33:56PM -0800, Dan Williams wrote:
> Alison Schofield wrote:
> [..]
> > Rather than conditionally including headers or dealing with platform
> > differences, replace both basename() usages with a new implementation
> > using strrchar to find the last '/' in the path and return everything
> > after it, or the whole string if no '/' is found.
> 
> This feels like it wants a helper rather than a new open-coded thing.
> For example, devpath_to_devname() is an existing similar helper.
happy face

> 
> Alternatively, just create a local helper called basename when the
> C-library is missing the GNU version.
grimace

Prepping the happy face version next.