Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use portable implementation for basename API #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kraj
Copy link
Contributor

@kraj kraj commented Dec 4, 2023

musl has removed the non-prototype declaration of basename from string.h [1] which now results in build errors with clang-17+ compiler

include libgen.h for using the posix declaration of the funciton.

Fixes
../git/tools/kmod.c:71:19: error: call to undeclared function 'basename'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
71 | "Commands:\n", basename(argv[0]));
| ^

[1] https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7
Signed-off-by: Khem Raj raj.khem@gmail.com

@lucasdemarchi
Copy link
Contributor

But they are not the same. See VERSIONS @ https://man7.org/linux/man-pages/man3/basename.3.html

We pass in a const pointer and expect it not to be modified by basename()

@kraj
Copy link
Contributor Author

kraj commented Dec 4, 2023

But they are not the same. See VERSIONS @ https://man7.org/linux/man-pages/man3/basename.3.html

We pass in a const pointer and expect it not to be modified by basename()

ok, I thought it was not the case, however if we want to emulate the glibc behaviour we will need to create a temporary string and use it as input to basename(). This will let it use posix basename. Will that be ok ?

@lucasdemarchi
Copy link
Contributor

I'd expect musl to have suggested change for this api breakage

@richfelker
Copy link

If the GNU basename behavior is really what's wanted, that can be achieved with basically a one-line function calling strrchr, something like:

static const char *gnu_basename(const char *s) {
    const char *p = strrchr(s, '/');
    return p ? p+1 : s;
}

@lucasdemarchi
Copy link
Contributor

Thanks @richfelker. Yeah, using that as alternative sounds good to me.

musl has removed the non-prototype declaration of basename from
string.h [1] which now results in build errors with clang-17+ compiler

Implement GNU basename behavior using strchr which is portable across libcs

Fixes
../git/tools/kmod.c:71:19: error: call to undeclared function 'basename'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
71 | "Commands:\n", basename(argv[0]));
| ^

[1] https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7

Suggested-by: Rich Felker

Signed-off-by: Khem Raj <raj.khem@gmail.com>
@kraj kraj changed the title include libgen.h for basename API Use portable implementation for basename API Dec 10, 2023
algitbot pushed a commit to alpinelinux/aports that referenced this pull request Feb 21, 2024
Upstream PR #32

musl has removed the non-prototype declaration of basename from
string.h [1] which now results in build errors with clang-17+
compiler.

kmod-project/kmod#32
bell-sw pushed a commit to bell-sw/alpaquita-aports that referenced this pull request Feb 22, 2024
[ commit e794d41d834c0d2b5a69c42f4ae5946156f26a01 ]

Upstream PR #32

musl has removed the non-prototype declaration of basename from
string.h [1] which now results in build errors with clang-17+
compiler.

kmod-project/kmod#32
@@ -794,7 +794,7 @@ static int conf_files_insert_sorted(struct kmod_ctx *ctx,
bool is_single = false;

if (name == NULL) {
name = basename(path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general approach in kmod is to consider the gnu-behavior as the desired one and add missing stuff for other libc's where appropriate. That way we don't have to keep patching kmod when the next .c uses basename() rather than gnu_basename().

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the GNU basename() behaviour is more desirable (and agree that it is), but the unfortunate fact is that the POSIX behaviour exists and is not going away; it's a bad move on GNU's part to have defined a symbol with a different prototype and a different behaviour from the POSIX one.

If I don't want the POSIX behaviour, I don't use basename(). I think that every project should do the same. I suggest that you go with gnu_basename(), and keep patching kmod because using basename() is suboptimal instead of keeping patching kmod because it breaks on some systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make it more readable for someone to make this difference clear rather subtle, but if you want to keep it obscure and assume basename is what glibc implements and not what posix defines, I will try to rework this to override basename when the system is not using glibc. Although, it will sound wrong for such non-gnu systems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal is we do this: https://github.com/kmod-project/kmod/commits/tip-basename/

877280d fixup! fixup! Use portable implementation for basename API
3325c9d fixup! Use portable implementation for basename API
81580c1 Use portable implementation for basename API

I think we can just squash my commits onto yours if you agree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably split the missing.h, but that can be done on top.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will work as long as the project never includes libgen.h. Which, true, it doesn't have a reason to.

@kdave
Copy link

kdave commented Apr 18, 2024

I fixed the basename semantics in btrfs-progs and got pointed to this PR for a fix. You may need to add handling of "/" as special case, this is mentioned in the manual page that basename is supposed to return that as "/", i.e. the strrchr() would return empty string.

@lucasdemarchi
Copy link
Contributor

lucasdemarchi commented Apr 29, 2024

@kdave thanks for reaching out. However this is not what I get. :

# glibc
/foo/bar/: 
/foo/bar: bar
/foo: foo
/: 
:

# musl  with special / handling
/foo/bar/: 
/foo/bar: bar
/foo: foo
/: /
:

Checking the man page, it explicitly calls out that in case of '/' it returns the empty string. Emphasis mine:

The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular also when it is "/".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants