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

[Marc Herbert <marc.herbert@linux.intel.com>]

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.