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.

Sign up for GitHub

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

Jump to bottom

Use portable implementation for basename API #32

Closed
wants to merge 1 commit into from
Closed

Use portable implementation for basename API #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion libkmod/libkmod-config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucasdemarchi are you happy with this patch or do you have comments that should be addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, applying it now. Thanks

name = gnu_basename(path);
is_single = true;
}

Expand Down
4 changes: 2 additions & 2 deletions shared/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,9 @@ char *modname_normalize(const char *modname, char buf[static PATH_MAX], size_t *

char *path_to_modname(const char *path, char buf[static PATH_MAX], size_t *len)
{
char *modname;
const char *modname;

modname = basename(path);
modname = gnu_basename(path);
if (modname == NULL || modname[0] == '\0')
return NULL;

Expand Down
7 changes: 7 additions & 0 deletions shared/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <stdbool.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <time.h>
Expand Down Expand Up @@ -76,6 +77,12 @@ do { \
__p->__v = (val); \
} while(0)

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

static _always_inline_ unsigned int ALIGN_POWER2(unsigned int u)
{
return 1 << ((sizeof(u) * 8) - __builtin_clz(u - 1));
Expand Down
2 changes: 1 addition & 1 deletion testsuite/testsuite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ static void help(void)

printf("Usage:\n"
"\t%s [options] <test>\n"
"Options:\n", basename(progname));
"Options:\n", gnu_basename(progname));

for (itr = options, itr_short = options_short;
itr->name != NULL; itr++, itr_short++)
Expand Down
2 changes: 1 addition & 1 deletion tools/depmod.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,7 @@ static int cfg_files_insert_sorted(struct cfg_file ***p_files, size_t *p_n_files
if (name != NULL)
namelen = strlen(name);
else {
name = basename(dir);
name = gnu_basename(dir);
namelen = strlen(name);
dirlen -= namelen + 1;
}
Expand Down
4 changes: 2 additions & 2 deletions tools/kmod.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ static int kmod_help(int argc, char *argv[])
"Options:\n"
"\t-V, --version show version\n"
"\t-h, --help show this help\n\n"
"Commands:\n", basename(argv[0]));
"Commands:\n", gnu_basename(argv[0]));

for (i = 0; i < ARRAY_SIZE(kmod_cmds); i++) {
if (kmod_cmds[i]->help != NULL) {
Expand Down Expand Up @@ -156,7 +156,7 @@ static int handle_kmod_compat_commands(int argc, char *argv[])
const char *cmd;
size_t i;

cmd = basename(argv[0]);
cmd = gnu_basename(argv[0]);

for (i = 0; i < ARRAY_SIZE(kmod_compat_cmds); i++) {
if (streq(kmod_compat_cmds[i]->name, cmd))
Expand Down