[PATCH 2/3] check if __GLIBC__ is defined before using _FILE_OFFSET_BITS

[John Spencer]

_FILE_OFFSET_BITS is a glibc internal macro.
musl provides 64bit off_t by default, but
defines stat64 etc as macros to make glibc-centric
programs happy. however the expansion causes
problems with the hack used here to work around
glibc's 32bit past...

Signed-off-by: John Spencer <maillist-kmod-***@public.gmane.org>

---
testsuite/path.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/testsuite/path.c b/testsuite/path.c
index f87f5c5..f06fadd 100644
--- a/testsuite/path.c
+++ b/testsuite/path.c
@@ -185,7 +185,7 @@ WRAP_2ARGS(int, -1, mkdir, mode_t);
WRAP_2ARGS(int, -1, access, int);
WRAP_2ARGS(int, -1, stat, struct stat*);
WRAP_2ARGS(int, -1, lstat, struct stat*);
-#ifndef _FILE_OFFSET_BITS
+#if defined(__GLIBC__) && !defined(_FILE_OFFSET_BITS)
WRAP_2ARGS(int, -1, stat64, struct stat64*);
WRAP_2ARGS(int, -1, lstat64, struct stat64*);
WRAP_OPEN(64);
@@ -196,7 +196,7 @@ WRAP_OPEN();
#ifdef HAVE___XSTAT
WRAP_VERSTAT(__x,);
WRAP_VERSTAT(__lx,);
-#ifndef _FILE_OFFSET_BITS
+#if defined(__GLIBC__) && !defined(_FILE_OFFSET_BITS)
WRAP_VERSTAT(__x,64);
WRAP_VERSTAT(__lx,64);
#endif

[John Spencer]

stdout and stderr are names reserved for the implementation
and musl uses them rightfully as macro - and the expansion
causes (of course) unexpected results.

renaming the struct members stdout to std_out and stderr
to std_err, to be 1) compliant 2) cause compilation to
succeed.

fixes build with musl libc.

Signed-off-by: John Spencer <maillist-kmod-***@public.gmane.org>

---
testsuite/test-alias.c | 2 +-
testsuite/test-loaded.c | 2 +-
testsuite/test-modinfo.c | 2 +-
testsuite/test-modprobe.c | 8 ++++----
testsuite/test-new-module.c | 4 ++--
testsuite/testsuite.c | 30 +++++++++++++++---------------
testsuite/testsuite.h | 4 ++--
7 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/testsuite/test-alias.c b/testsuite/test-alias.c
index 5e21573..a4b4c14 100644
--- a/testsuite/test-alias.c
+++ b/testsuite/test-alias.c
@@ -64,7 +64,7 @@ static DEFINE_TEST(alias_1,
},
.need_spawn = true,
.output = {
- .stdout = TESTSUITE_ROOTFS "test-alias/correct.txt",
+ .std_out = TESTSUITE_ROOTFS "test-alias/correct.txt",
});

static const struct test *tests[] = {
diff --git a/testsuite/test-loaded.c b/testsuite/test-loaded.c
index 33865cb..734a0a8 100644
--- a/testsuite/test-loaded.c
+++ b/testsuite/test-loaded.c
@@ -85,7 +85,7 @@ static DEFINE_TEST(loaded_1,
},
.need_spawn = true,
.output = {
- .stdout = TESTSUITE_ROOTFS "test-loaded/correct.txt",
+ .std_out = TESTSUITE_ROOTFS "test-loaded/correct.txt",
});

static const struct test *tests[] = {
diff --git a/testsuite/test-modinfo.c b/testsuite/test-modinfo.c
index c5934ea..c387633 100644
--- a/testsuite/test-modinfo.c
+++ b/testsuite/test-modinfo.c
@@ -46,7 +46,7 @@ static DEFINE_TEST(modinfo_jonsmodules,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modinfo/",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modinfo/correct.txt",
+ .std_out = TESTSUITE_ROOTFS "test-modinfo/correct.txt",
});

static const struct test *tests[] = {
diff --git a/testsuite/test-modprobe.c b/testsuite/test-modprobe.c
index f0131e3..5dfbe22 100644
--- a/testsuite/test-modprobe.c
+++ b/testsuite/test-modprobe.c
@@ -45,7 +45,7 @@ static DEFINE_TEST(modprobe_show_depends,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/show-depends",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct.txt",
+ .std_out = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct.txt",
});

static __noreturn int modprobe_show_depends2(const struct test *t)
@@ -67,7 +67,7 @@ static DEFINE_TEST(modprobe_show_depends2,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/show-depends",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct-psmouse.txt",
+ .std_out = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct-psmouse.txt",
});


@@ -90,7 +90,7 @@ static DEFINE_TEST(modprobe_show_alias_to_none,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/alias-to-none",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct-psmouse.txt",
+ .std_out = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct-psmouse.txt",
});


@@ -177,7 +177,7 @@ static DEFINE_TEST(modprobe_param_kcmdline,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline/correct.txt",
+ .std_out = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline/correct.txt",
});


diff --git a/testsuite/test-new-module.c b/testsuite/test-new-module.c
index 240480e..cb603d4 100644
--- a/testsuite/test-new-module.c
+++ b/testsuite/test-new-module.c
@@ -67,7 +67,7 @@ static DEFINE_TEST(from_name,
},
.need_spawn = true,
.output = {
- .stdout = TESTSUITE_ROOTFS "test-new-module/from_name/correct.txt",
+ .std_out = TESTSUITE_ROOTFS "test-new-module/from_name/correct.txt",
});

static int from_alias(const struct test *t)
@@ -112,7 +112,7 @@ static DEFINE_TEST(from_alias,
},
.need_spawn = true,
.output = {
- .stdout = TESTSUITE_ROOTFS "test-new-module/from_alias/correct.txt",
+ .std_out = TESTSUITE_ROOTFS "test-new-module/from_alias/correct.txt",
});

static const struct test *tests[] = {
diff --git a/testsuite/testsuite.c b/testsuite/testsuite.c
index 6d85d7f..0b66cb9 100644
--- a/testsuite/testsuite.c
+++ b/testsuite/testsuite.c
@@ -223,7 +223,7 @@ static inline int test_run_child(const struct test *t, int fdout[2],
test_export_environ(t);

/* Close read-fds and redirect std{out,err} to the write-fds */
- if (t->output.stdout != NULL) {
+ if (t->output.std_out != NULL) {
close(fdout[0]);
if (dup2(fdout[1], STDOUT_FILENO) < 0) {
ERR("could not redirect stdout to pipe: %m\n");
@@ -231,10 +231,10 @@ static inline int test_run_child(const struct test *t, int fdout[2],
}
}

- if (t->output.stderr != NULL) {
+ if (t->output.std_err != NULL) {
close(fderr[0]);
if (dup2(fderr[1], STDERR_FILENO) < 0) {
- ERR("could not redirect stdout to pipe: %m\n");
+ ERR("could not redirect stderr to pipe: %m\n");
exit(EXIT_FAILURE);
}
}
@@ -282,12 +282,12 @@ static inline bool test_run_parent_check_outputs(const struct test *t,
return false;
}

- if (t->output.stdout != NULL) {
- fd_matchout = open(t->output.stdout, O_RDONLY);
+ if (t->output.std_out != NULL) {
+ fd_matchout = open(t->output.std_out, O_RDONLY);
if (fd_matchout < 0) {
err = -errno;
ERR("could not open %s for read: %m\n",
- t->output.stdout);
+ t->output.std_out);
goto out;
}
memset(&ep_outpipe, 0, sizeof(struct epoll_event));
@@ -301,12 +301,12 @@ static inline bool test_run_parent_check_outputs(const struct test *t,
} else
fdout = -1;

- if (t->output.stderr != NULL) {
- fd_matcherr = open(t->output.stderr, O_RDONLY);
+ if (t->output.std_err != NULL) {
+ fd_matcherr = open(t->output.std_err, O_RDONLY);
if (fd_matcherr < 0) {
err = -errno;
ERR("could not open %s for read: %m\n",
- t->output.stderr);
+ t->output.std_err);
goto out;

}
@@ -536,9 +536,9 @@ static inline int test_run_parent(const struct test *t, int fdout[2],
bool matchout;

/* Close write-fds */
- if (t->output.stdout != NULL)
+ if (t->output.std_out != NULL)
close(fdout[1]);
- if (t->output.stderr != NULL)
+ if (t->output.std_err != NULL)
close(fderr[1]);
close(fdmonitor[1]);

@@ -549,9 +549,9 @@ static inline int test_run_parent(const struct test *t, int fdout[2],
* break pipe on the other end: either child already closed or we want
* to stop it
*/
- if (t->output.stdout != NULL)
+ if (t->output.std_out != NULL)
close(fdout[0]);
- if (t->output.stderr != NULL)
+ if (t->output.std_err != NULL)
close(fderr[0]);
close(fdmonitor[0]);

@@ -650,14 +650,14 @@ int test_run(const struct test *t)
if (t->need_spawn && oneshot)
test_run_spawned(t);

- if (t->output.stdout != NULL) {
+ if (t->output.std_out != NULL) {
if (pipe(fdout) != 0) {
ERR("could not create out pipe for %s\n", t->name);
return EXIT_FAILURE;
}
}

- if (t->output.stderr != NULL) {
+ if (t->output.std_err != NULL) {
if (pipe(fderr) != 0) {
ERR("could not create err pipe for %s\n", t->name);
return EXIT_FAILURE;
diff --git a/testsuite/testsuite.h b/testsuite/testsuite.h
index 329d4a1..f8085a7 100644
--- a/testsuite/testsuite.h
+++ b/testsuite/testsuite.h
@@ -82,9 +82,9 @@ struct test {
const char *description;
struct {
/* File with correct stdout */
- const char *stdout;
+ const char *std_out;
/* File with correct stderr */
- const char *stderr;
+ const char *std_err;

/*
* Vector with pair of files

[Lucas De Marchi]

Post by John Spencer
stdout and stderr are names reserved for the implementation
and musl uses them rightfully as macro - and the expansion
causes (of course) unexpected results.

couldn't musl just do what glibc does here?

<... define the types here ...>
#define stdin stdin
#define stdout stdout
#define stderr stderr

This would avoid patches like this to several projects.

Post by John Spencer
renaming the struct members stdout to std_out and stderr
to std_err, to be 1) compliant 2) cause compilation to
succeed.

uggh.. "out" and "err" otherwise. No need for the "std_" prefix.


Lucas De Marchi

[John Spencer]

stdout and stderr are names reserved for the implementation
and musl uses them rightfully as macro - and the expansion
causes (of course) unexpected results.

rename the struct members stdout to out and stderr
to err, to be 1) compliant 2) cause compilation to
succeed.

fixes build with musl libc.

Signed-off-by: John Spencer <maillist-kmod-***@public.gmane.org>

---
testsuite/test-alias.c | 2 +-
testsuite/test-loaded.c | 2 +-
testsuite/test-modinfo.c | 2 +-
testsuite/test-modprobe.c | 8 ++++----
testsuite/test-new-module.c | 4 ++--
testsuite/testsuite.c | 30 +++++++++++++++---------------
testsuite/testsuite.h | 4 ++--
7 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/testsuite/test-alias.c b/testsuite/test-alias.c
index 5e21573..4d1d8d3 100644
--- a/testsuite/test-alias.c
+++ b/testsuite/test-alias.c
@@ -64,7 +64,7 @@ static DEFINE_TEST(alias_1,
},
.need_spawn = true,
.output = {
- .stdout = TESTSUITE_ROOTFS "test-alias/correct.txt",
+ .out = TESTSUITE_ROOTFS "test-alias/correct.txt",
});

static const struct test *tests[] = {
diff --git a/testsuite/test-loaded.c b/testsuite/test-loaded.c
index 33865cb..4b50813 100644
--- a/testsuite/test-loaded.c
+++ b/testsuite/test-loaded.c
@@ -85,7 +85,7 @@ static DEFINE_TEST(loaded_1,
},
.need_spawn = true,
.output = {
- .stdout = TESTSUITE_ROOTFS "test-loaded/correct.txt",
+ .out = TESTSUITE_ROOTFS "test-loaded/correct.txt",
});

static const struct test *tests[] = {
diff --git a/testsuite/test-modinfo.c b/testsuite/test-modinfo.c
index c5934ea..e372310 100644
--- a/testsuite/test-modinfo.c
+++ b/testsuite/test-modinfo.c
@@ -46,7 +46,7 @@ static DEFINE_TEST(modinfo_jonsmodules,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modinfo/",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modinfo/correct.txt",
+ .out = TESTSUITE_ROOTFS "test-modinfo/correct.txt",
});

static const struct test *tests[] = {
diff --git a/testsuite/test-modprobe.c b/testsuite/test-modprobe.c
index f0131e3..0ae03a6 100644
--- a/testsuite/test-modprobe.c
+++ b/testsuite/test-modprobe.c
@@ -45,7 +45,7 @@ static DEFINE_TEST(modprobe_show_depends,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/show-depends",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct.txt",
+ .out = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct.txt",
});

static __noreturn int modprobe_show_depends2(const struct test *t)
@@ -67,7 +67,7 @@ static DEFINE_TEST(modprobe_show_depends2,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/show-depends",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct-psmouse.txt",
+ .out = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct-psmouse.txt",
});


@@ -90,7 +90,7 @@ static DEFINE_TEST(modprobe_show_alias_to_none,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/alias-to-none",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct-psmouse.txt",
+ .out = TESTSUITE_ROOTFS "test-modprobe/show-depends/correct-psmouse.txt",
});


@@ -177,7 +177,7 @@ static DEFINE_TEST(modprobe_param_kcmdline,
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline",
},
.output = {
- .stdout = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline/correct.txt",
+ .out = TESTSUITE_ROOTFS "test-modprobe/module-param-kcmdline/correct.txt",
});


diff --git a/testsuite/test-new-module.c b/testsuite/test-new-module.c
index 240480e..0892507 100644
--- a/testsuite/test-new-module.c
+++ b/testsuite/test-new-module.c
@@ -67,7 +67,7 @@ static DEFINE_TEST(from_name,
},
.need_spawn = true,
.output = {
- .stdout = TESTSUITE_ROOTFS "test-new-module/from_name/correct.txt",
+ .out = TESTSUITE_ROOTFS "test-new-module/from_name/correct.txt",
});

static int from_alias(const struct test *t)
@@ -112,7 +112,7 @@ static DEFINE_TEST(from_alias,
},
.need_spawn = true,
.output = {
- .stdout = TESTSUITE_ROOTFS "test-new-module/from_alias/correct.txt",
+ .out = TESTSUITE_ROOTFS "test-new-module/from_alias/correct.txt",
});

static const struct test *tests[] = {
diff --git a/testsuite/testsuite.c b/testsuite/testsuite.c
index 6d85d7f..9877a64 100644
--- a/testsuite/testsuite.c
+++ b/testsuite/testsuite.c
@@ -223,7 +223,7 @@ static inline int test_run_child(const struct test *t, int fdout[2],
test_export_environ(t);

/* Close read-fds and redirect std{out,err} to the write-fds */
- if (t->output.stdout != NULL) {
+ if (t->output.out != NULL) {
close(fdout[0]);
if (dup2(fdout[1], STDOUT_FILENO) < 0) {
ERR("could not redirect stdout to pipe: %m\n");
@@ -231,10 +231,10 @@ static inline int test_run_child(const struct test *t, int fdout[2],
}
}

- if (t->output.stderr != NULL) {
+ if (t->output.err != NULL) {
close(fderr[0]);
if (dup2(fderr[1], STDERR_FILENO) < 0) {
- ERR("could not redirect stdout to pipe: %m\n");
+ ERR("could not redirect stderr to pipe: %m\n");
exit(EXIT_FAILURE);
}
}
@@ -282,12 +282,12 @@ static inline bool test_run_parent_check_outputs(const struct test *t,
return false;
}

- if (t->output.stdout != NULL) {
- fd_matchout = open(t->output.stdout, O_RDONLY);
+ if (t->output.out != NULL) {
+ fd_matchout = open(t->output.out, O_RDONLY);
if (fd_matchout < 0) {
err = -errno;
ERR("could not open %s for read: %m\n",
- t->output.stdout);
+ t->output.out);
goto out;
}
memset(&ep_outpipe, 0, sizeof(struct epoll_event));
@@ -301,12 +301,12 @@ static inline bool test_run_parent_check_outputs(const struct test *t,
} else
fdout = -1;

- if (t->output.stderr != NULL) {
- fd_matcherr = open(t->output.stderr, O_RDONLY);
+ if (t->output.err != NULL) {
+ fd_matcherr = open(t->output.err, O_RDONLY);
if (fd_matcherr < 0) {
err = -errno;
ERR("could not open %s for read: %m\n",
- t->output.stderr);
+ t->output.err);
goto out;

}
@@ -536,9 +536,9 @@ static inline int test_run_parent(const struct test *t, int fdout[2],
bool matchout;

/* Close write-fds */
- if (t->output.stdout != NULL)
+ if (t->output.out != NULL)
close(fdout[1]);
- if (t->output.stderr != NULL)
+ if (t->output.err != NULL)
close(fderr[1]);
close(fdmonitor[1]);

@@ -549,9 +549,9 @@ static inline int test_run_parent(const struct test *t, int fdout[2],
* break pipe on the other end: either child already closed or we want
* to stop it
*/
- if (t->output.stdout != NULL)
+ if (t->output.out != NULL)
close(fdout[0]);
- if (t->output.stderr != NULL)
+ if (t->output.err != NULL)
close(fderr[0]);
close(fdmonitor[0]);

@@ -650,14 +650,14 @@ int test_run(const struct test *t)
if (t->need_spawn && oneshot)
test_run_spawned(t);

- if (t->output.stdout != NULL) {
+ if (t->output.out != NULL) {
if (pipe(fdout) != 0) {
ERR("could not create out pipe for %s\n", t->name);
return EXIT_FAILURE;
}
}

- if (t->output.stderr != NULL) {
+ if (t->output.err != NULL) {
if (pipe(fderr) != 0) {
ERR("could not create err pipe for %s\n", t->name);
return EXIT_FAILURE;
diff --git a/testsuite/testsuite.h b/testsuite/testsuite.h
index 329d4a1..e4f3ecf 100644
--- a/testsuite/testsuite.h
+++ b/testsuite/testsuite.h
@@ -82,9 +82,9 @@ struct test {
const char *description;
struct {
/* File with correct stdout */
- const char *stdout;
+ const char *out;
/* File with correct stderr */
- const char *stderr;
+ const char *err;

/*
* Vector with pair of files

[John Spencer]

Post by Lucas De Marchi

Post by John Spencer
stdout and stderr are names reserved for the implementation
and musl uses them rightfully as macro - and the expansion
causes (of course) unexpected results.

couldn't musl just do what glibc does here?

maybe, but often what glibc does is suboptimal or non-conforming, and
relying on some assumptions about how libc defines certain internal
specifiers (possibly temporary) seems pretty fragile. even glibc may
change its definition at some point to something that breaks kmod.

Post by Lucas De Marchi
<... define the types here ...>
#define stdin stdin
#define stdout stdout
#define stderr stderr
This would avoid patches like this to several projects.

i've ported about 500 packages to musl and this is first usage of stderr
and stdout as variable names i've seen so far.

Post by Lucas De Marchi

Post by John Spencer
renaming the struct members stdout to std_out and stderr
to std_err, to be 1) compliant 2) cause compilation to
succeed.

uggh.. "out" and "err" otherwise. No need for the "std_" prefix.

right, that looks much nicer now. updated patch sent separately.

Post by Lucas De Marchi
Lucas De Marchi

thanks,
--JS

[Lucas De Marchi]

Post by John Spencer
stdout and stderr are names reserved for the implementation
and musl uses them rightfully as macro - and the expansion
causes (of course) unexpected results.
renaming the struct members stdout to std_out and stderr
to std_err, to be 1) compliant 2) cause compilation to
succeed.
fixes build with musl libc.

We don't use signed-of-by in kmod.

Since this was the only issue I went ahead and applied the patch without it.


Thanks.

Lucas De Marchi

[Kay Sievers]

usage of strndupa is neither C99 nor POSIX,
so musl libc does not implement it.
it's a glibc invention, and a dangerous one since
usage of alloca() is considered bad practice.

It's as dangerous as variable sized arrays, and surely much less "bad
practice" than your introduced PATH_MAX. :)

If kmod declares posix-only interfaces as the goal, then it's fine,
but your reasons do not make much sense in 2013.

Kay

[Lucas De Marchi]

Post by John Spencer
_FILE_OFFSET_BITS is a glibc internal macro.
musl provides 64bit off_t by default, but
defines stat64 etc as macros to make glibc-centric
programs happy. however the expansion causes
problems with the hack used here to work around
glibc's 32bit past...

AFAIK this define comes from AC_SYS_LARGEFILE in autoconf and ends up
in our config.h. Checking for the libc in source code is not much
future proof and I'd like to avoid it. If this is not the right
define to pick, is there any other one?

Maybe it would be better to do like this:

#ifndef _FILE_OFFSET_BITS
#ifndef stat64
WRAP_2ARGS(int, -1, stat64, struct stat64*);
#endif
...

Because if it's a macro, there's no need to wrap the call.

Lucas De Marchi

[John Spencer]

Post by Lucas De Marchi

Post by John Spencer
_FILE_OFFSET_BITS is a glibc internal macro.
musl provides 64bit off_t by default, but
defines stat64 etc as macros to make glibc-centric
programs happy. however the expansion causes
problems with the hack used here to work around
glibc's 32bit past...

AFAIK this define comes from AC_SYS_LARGEFILE in autoconf and ends up
in our config.h. Checking for the libc in source code is not much
future proof and I'd like to avoid it. If this is not the right
define to pick, is there any other one?
#ifndef _FILE_OFFSET_BITS
#ifndef stat64
WRAP_2ARGS(int, -1, stat64, struct stat64*);
#endif
...
Because if it's a macro, there's no need to wrap the call.

Yes, that would work as well.

Post by Lucas De Marchi
Lucas De Marchi

Thanks,
--JS

[Lucas De Marchi]

usage of strndupa is neither C99 nor POSIX,
so musl libc does not implement it.
it's a glibc invention, and a dangerous one since
usage of alloca() is considered bad practice.

If the input is already sanitized and checked for length, this is
isn't really dangerous. Particularly on recursive functions this also
avoids growing the stack much more than needed.

fixes build with musl libc.

for me it seems more like an excuse to not implement it in musl.
Particularly because there are other places in which we call alloca(),
that you didn't complain. What does musl do there? If musl has
alloca(), doing strndupa in missing.h would be doable.

---
libkmod/libkmod-util.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/libkmod/libkmod-util.c b/libkmod/libkmod-util.c
index d686250..9615d0f 100644
--- a/libkmod/libkmod-util.c
+++ b/libkmod/libkmod-util.c
@@ -28,6 +28,7 @@
#include <unistd.h>
#include <errno.h>
#include <string.h>
+#include <limits.h>
#include <ctype.h>
#include "libkmod.h"
@@ -323,8 +324,11 @@ static inline int is_dir(const char *path)
int mkdir_p(const char *path, int len, mode_t mode)
{
char *start, *end;
-
- start = strndupa(path, len);
+ char buf[PATH_MAX+1];
+ snprintf(buf, sizeof buf, "%s", path);

snprintf to dup a string? You already know the len. memcpy would be way simpler

Lucas De Marchi

[John Spencer]

Post by Lucas De Marchi

usage of strndupa is neither C99 nor POSIX,
so musl libc does not implement it.
it's a glibc invention, and a dangerous one since
usage of alloca() is considered bad practice.

If the input is already sanitized and checked for length, this is
isn't really dangerous. Particularly on recursive functions this also
avoids growing the stack much more than needed.

yes, but it is a dangerous function which can and will be misused when
it is provided.

Post by Lucas De Marchi

fixes build with musl libc.

for me it seems more like an excuse to not implement it in musl.
Particularly because there are other places in which we call alloca(),
that you didn't complain. What does musl do there? If musl has
alloca(), doing strndupa in missing.h would be doable.

i just fixed strndupa usage in eudev's mkdir_p function a week ago, and
that was the first usage of that function i've seen so far in ~500
packages i've ported. the function used by kmod seems to be derived from
that one, since the context, name and everything else are nearly identical.

musl usually doesn't add exotic functions only used by a single package,
even moreso when the function in question can compromise security.

since kmod is not a red-hat-GNU-linux specific package like systemd
(which is even proud of nonportable code), but a linux-specific one,
portability at least among available linux libcs should be a concern.

Post by Lucas De Marchi

---
libkmod/libkmod-util.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/libkmod/libkmod-util.c b/libkmod/libkmod-util.c
index d686250..9615d0f 100644
--- a/libkmod/libkmod-util.c
+++ b/libkmod/libkmod-util.c
@@ -28,6 +28,7 @@
#include<unistd.h>
#include<errno.h>
#include<string.h>
+#include<limits.h>
#include<ctype.h>
#include "libkmod.h"
@@ -323,8 +324,11 @@ static inline int is_dir(const char *path)
int mkdir_p(const char *path, int len, mode_t mode)
{
char *start, *end;
-
- start = strndupa(path, len);
+ char buf[PATH_MAX+1];
+ snprintf(buf, sizeof buf, "%s", path);

snprintf to dup a string? You already know the len. memcpy would be way simpler

this is to cover the potential case where len > strlen(path).
there's no documentation indicating this case cannot happen.
otherwise an out-of-bounds read would happen which could potentially
cause a segfault. i suspect this function is not called million of times
in an inner loop, so i opted for security over speed.

Post by Lucas De Marchi
Lucas De Marchi

[Kay Sievers]

Post by Lucas De Marchi

usage of strndupa is neither C99 nor POSIX,
so musl libc does not implement it.
it's a glibc invention, and a dangerous one since
usage of alloca() is considered bad practice.

If the input is already sanitized and checked for length, this is
isn't really dangerous. Particularly on recursive functions this also
avoids growing the stack much more than needed.

yes, but it is a dangerous function which can and will be misused when it is
provided.

Dangerous? Misused? That is just pure nonsense.

musl usually doesn't add exotic functions only used by a single package,
even moreso when the function in question can compromise security.

So, this exotic libc can't be used for kmod as it is. Fine. :)

since kmod is not a red-hat-GNU-linux specific package like systemd (which
is even proud of nonportable code), but a linux-specific one, portability at
least among available linux libcs should be a concern.

Just a hint: I would try to be less confrontational, if you like stuff
to be changed in a way that matters only to you.

Kay

[Lucas De Marchi]

Post by Lucas De Marchi

usage of strndupa is neither C99 nor POSIX,
so musl libc does not implement it.
it's a glibc invention, and a dangerous one since
usage of alloca() is considered bad practice.

If the input is already sanitized and checked for length, this is
isn't really dangerous. Particularly on recursive functions this also
avoids growing the stack much more than needed.

yes, but it is a dangerous function which can and will be misused when it is
provided.

If I wanted a language that doesn't allow me to shoot myself on the
foot, I'd rather be writing pascal, basic or some other new
<name-your-preferred-script-language>

Post by Lucas De Marchi

fixes build with musl libc.

for me it seems more like an excuse to not implement it in musl.
Particularly because there are other places in which we call alloca(),
that you didn't complain. What does musl do there? If musl has
alloca(), doing strndupa in missing.h would be doable.

i just fixed strndupa usage in eudev's mkdir_p function a week ago, and that
was the first usage of that function i've seen so far in ~500 packages i've
ported. the function used by kmod seems to be derived from that one, since
the context, name and everything else are nearly identical.

apart from the name, I don't think it's derived

musl usually doesn't add exotic functions only used by a single package,

if you keep removing it from the packages, you may decrease the number
of users.... but counting now you just said you have 2. And I can name
some others, too.

even moreso when the function in question can compromise security.
since kmod is not a red-hat-GNU-linux specific package like systemd (which
is even proud of nonportable code), but a linux-specific one, portability at
least among available linux libcs should be a concern.

yes, I care. I do care other packages to be high quality too instead
of just throwing into kmod an insane amount of ifdefs and friends to
support everything. In the past this worked very well and allowed us
to move forward faster and link to bionic, dielibc, klibc and musl. I
appreciate patches adding support to other libc, but they have to be
reasonable.

Post by Lucas De Marchi

---
libkmod/libkmod-util.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/libkmod/libkmod-util.c b/libkmod/libkmod-util.c
index d686250..9615d0f 100644
--- a/libkmod/libkmod-util.c
+++ b/libkmod/libkmod-util.c
@@ -28,6 +28,7 @@
#include<unistd.h>
#include<errno.h>
#include<string.h>
+#include<limits.h>
#include<ctype.h>
#include "libkmod.h"
@@ -323,8 +324,11 @@ static inline int is_dir(const char *path)
int mkdir_p(const char *path, int len, mode_t mode)
{
char *start, *end;
-
- start = strndupa(path, len);
+ char buf[PATH_MAX+1];
+ snprintf(buf, sizeof buf, "%s", path);

snprintf to dup a string? You already know the len. memcpy would be way simpler

this is to cover the potential case where len > strlen(path).
there's no documentation indicating this case cannot happen.

there are only 2 callers of this function. Copying the entire string
here is actually the wrong thing to do.

Back to the subject, musl does provide alloca(), which is the
"dangerous" function according to you. If you or musl devs don't want
to provide strndupa(), the patch to be accepted in kmod is the one
that checks if strndupa() is available and add it to missing.h
otherwise. Why missing.h? To remember me to check it some time in
future to see if I can already remove some stuff.


Lucas De Marchi

[John Spencer]

Post by Lucas De Marchi

Post by Lucas De Marchi

usage of strndupa is neither C99 nor POSIX,
so musl libc does not implement it.
it's a glibc invention, and a dangerous one since
usage of alloca() is considered bad practice.

If the input is already sanitized and checked for length, this is
isn't really dangerous. Particularly on recursive functions this also
avoids growing the stack much more than needed.

yes, but it is a dangerous function which can and will be misused when it is
provided.

If I wanted a language that doesn't allow me to shoot myself on the
foot, I'd rather be writing pascal, basic or some other new
<name-your-preferred-script-language>

yes, but there's a reason alloca is not in the C standard and gets() got
deprecated.

Post by Lucas De Marchi

Post by Lucas De Marchi

fixes build with musl libc.

for me it seems more like an excuse to not implement it in musl.
Particularly because there are other places in which we call alloca(),
that you didn't complain. What does musl do there? If musl has
alloca(), doing strndupa in missing.h would be doable.

i just fixed strndupa usage in eudev's mkdir_p function a week ago, and that
was the first usage of that function i've seen so far in ~500 packages i've
ported. the function used by kmod seems to be derived from that one, since
the context, name and everything else are nearly identical.

apart from the name, I don't think it's derived

musl usually doesn't add exotic functions only used by a single package,

if you keep removing it from the packages, you may decrease the number
of users.... but counting now you just said you have 2. And I can name
some others, too.

no, i have one, since eudev merged my patch without turning a hair.
they didn't fight around just to keep their single strndupa call in some
legacy systemd derived code.
the others you can name are most likely udev and systemd, which i don't
intend to ever compile, same as pulseaudio and other poettering crapware.

Post by Lucas De Marchi

even moreso when the function in question can compromise security.
since kmod is not a red-hat-GNU-linux specific package like systemd (which
is even proud of nonportable code), but a linux-specific one, portability at
least among available linux libcs should be a concern.

yes, I care. I do care other packages to be high quality too instead
of just throwing into kmod an insane amount of ifdefs and friends to
support everything. In the past this worked very well and allowed us
to move forward faster and link to bionic, dielibc, klibc and musl. I
appreciate patches adding support to other libc, but they have to be
reasonable.

high quality, high portability, no ifdefs - this is exactly what the
proposed patch here does.

what exactly do you find non-reasonable regarding this patch ?

kmod used to build just fine in the past, you're the one who broke it
last month by throwing the mkdir_p code into the source tree.
and the function has seen quite some refactoring already, using strdupa,
then strndupa. who guarantees me that if i add strndupa (assuming rich
would accept and merge my patch) to musl, you won't change your code
back to use strdupa in the next release ?

Post by Lucas De Marchi

Post by Lucas De Marchi

---
libkmod/libkmod-util.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/libkmod/libkmod-util.c b/libkmod/libkmod-util.c
index d686250..9615d0f 100644
--- a/libkmod/libkmod-util.c
+++ b/libkmod/libkmod-util.c
@@ -28,6 +28,7 @@
#include<unistd.h>
#include<errno.h>
#include<string.h>
+#include<limits.h>
#include<ctype.h>
#include "libkmod.h"
@@ -323,8 +324,11 @@ static inline int is_dir(const char *path)
int mkdir_p(const char *path, int len, mode_t mode)
{
char *start, *end;
-
- start = strndupa(path, len);
+ char buf[PATH_MAX+1];
+ snprintf(buf, sizeof buf, "%s", path);

snprintf to dup a string? You already know the len. memcpy would be way simpler

this is to cover the potential case where len> strlen(path).
there's no documentation indicating this case cannot happen.

there are only 2 callers of this function. Copying the entire string
here is actually the wrong thing to do.
Back to the subject, musl does provide alloca(), which is the
"dangerous" function according to you. If you or musl devs don't want
to provide strndupa(), the patch to be accepted in kmod is the one
that checks if strndupa() is available and add it to missing.h
otherwise. Why missing.h? To remember me to check it some time in
future to see if I can already remove some stuff.

actually i didn't even ask yet if we can ask strndupa, because it was
first encountered a week back in said eudev code, and my patch got
immediately merged.
when i see non-portable code, i fix the nonportable code, not libc, and
send a patch to upstream in the hope that it gets merged. if they don't
merge it, ok - yet another fork caused by maintainer stubborness.
how do you like "kemod" as a name for the fork ?

yes, alloca() was added as it is widely used - you can't even build gcc
without it. the same can't be said about strndupa which is only used in
one mkdir code snippet copy-pasted from udev into a handful of projects.

besides, afaik strndupa can't even be implemented as a function, since
it would return a pointer to automatic storage that's already out of
scope (at least not without using asm).

that said, i am not sending you a macro replacement for a missing
strndupa and doing a full stop now. if you care about the musl userbase,
merge this patch, otherwise ignore it and live with a fork.

Post by Lucas De Marchi
Lucas De Marchi

P.S. i don't even use kmod myself (contrary to what poettering's twin
sievers believes), i ported this quickly for usage in dragora linux
which is just switching from glibc to musl and the maintainer asked for
help.
there will soon be a musl-based gentoo as well btw, and next version of
alpine will be musl based as well.

[Lucas De Marchi]

Hi John,

Post by John Spencer

Post by Lucas De Marchi

Post by Lucas De Marchi

usage of strndupa is neither C99 nor POSIX,
so musl libc does not implement it.
it's a glibc invention, and a dangerous one since
usage of alloca() is considered bad practice.

If the input is already sanitized and checked for length, this is
isn't really dangerous. Particularly on recursive functions this also
avoids growing the stack much more than needed.

yes, but it is a dangerous function which can and will be misused when it is
provided.

If I wanted a language that doesn't allow me to shoot myself on the
foot, I'd rather be writing pascal, basic or some other new
<name-your-preferred-script-language>

yes, but there's a reason alloca is not in the C standard and gets() got
deprecated.

the thing is... a standard is a live thing. New things are added.
Other things are removed. gets() was removed because there's no way to
use it correctly. New things are being added because people need and
because we are getting innovative things being done.

If we were to limit ourselves only to things that were already in a
standard, the standard would never need to be updated and how boring
would be life.

Post by John Spencer

Post by Lucas De Marchi

Post by Lucas De Marchi

fixes build with musl libc.

for me it seems more like an excuse to not implement it in musl.
Particularly because there are other places in which we call alloca(),
that you didn't complain. What does musl do there? If musl has
alloca(), doing strndupa in missing.h would be doable.

i just fixed strndupa usage in eudev's mkdir_p function a week ago, and that
was the first usage of that function i've seen so far in ~500 packages i've
ported. the function used by kmod seems to be derived from that one, since
the context, name and everything else are nearly identical.

apart from the name, I don't think it's derived

musl usually doesn't add exotic functions only used by a single package,

if you keep removing it from the packages, you may decrease the number
of users.... but counting now you just said you have 2. And I can name
some others, too.

no, i have one, since eudev merged my patch without turning a hair.
they didn't fight around just to keep their single strndupa call in some
legacy systemd derived code.
the others you can name are most likely udev and systemd, which i don't
intend to ever compile, same as pulseaudio and other poettering crapware.

some projects are different and reviewers are picker about the changes
merged. If you are aiming to make several projects to build correctly
with musl, the best thing you do is to adapt yourself to the different
projects.

Strong words and bashing other projects won't help your cause.

Post by John Spencer

Post by Lucas De Marchi

even moreso when the function in question can compromise security.
since kmod is not a red-hat-GNU-linux specific package like systemd (which
is even proud of nonportable code), but a linux-specific one, portability at
least among available linux libcs should be a concern.

yes, I care. I do care other packages to be high quality too instead
of just throwing into kmod an insane amount of ifdefs and friends to
support everything. In the past this worked very well and allowed us
to move forward faster and link to bionic, dielibc, klibc and musl. I
appreciate patches adding support to other libc, but they have to be
reasonable.

high quality, high portability, no ifdefs - this is exactly what the
proposed patch here does.
what exactly do you find non-reasonable regarding this patch ?

Because we/I like extensions and other projects could be improved by
not getting stuck on old things. GCC was improved because of that.
LLVM was improved because of that. Several libc's were improved
because of that.

I don't really mind if that particular piece of code is changed to a
fixed array. That function could be refactored to even not duplicate
each part of the path. However what I really want is people to *stop*
and *think* if other projects like musl could be improved instead of
just changing that piece of code.

For example, you didn't properly respond this particular question: if
musl already provides alloca(), why can't it provide str[n]dupa()? It
could be trivially done by a macro or static always inline function.

Post by John Spencer
kmod used to build just fine in the past, you're the one who broke it last
month by throwing the mkdir_p code into the source tree.
and the function has seen quite some refactoring already, using strdupa,
then strndupa. who guarantees me that if i add strndupa (assuming rich would
accept and merge my patch) to musl, you won't change your code back to use
strdupa in the next release ?

None. And I can't guarantee you next release will be bug free neither.
I don't use musl myself as I don't use plenty of other libc's or
architectures in which kmod runs on. This is one reason why we
appreciate bug reports and patches from others

Post by John Spencer

Post by Lucas De Marchi

Post by Lucas De Marchi

---
libkmod/libkmod-util.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/libkmod/libkmod-util.c b/libkmod/libkmod-util.c
index d686250..9615d0f 100644
--- a/libkmod/libkmod-util.c
+++ b/libkmod/libkmod-util.c
@@ -28,6 +28,7 @@
#include<unistd.h>
#include<errno.h>
#include<string.h>
+#include<limits.h>
#include<ctype.h>
#include "libkmod.h"
@@ -323,8 +324,11 @@ static inline int is_dir(const char *path)
int mkdir_p(const char *path, int len, mode_t mode)
{
char *start, *end;
-
- start = strndupa(path, len);
+ char buf[PATH_MAX+1];
+ snprintf(buf, sizeof buf, "%s", path);

snprintf to dup a string? You already know the len. memcpy would be way simpler

this is to cover the potential case where len> strlen(path).
there's no documentation indicating this case cannot happen.

there are only 2 callers of this function. Copying the entire string
here is actually the wrong thing to do.
Back to the subject, musl does provide alloca(), which is the
"dangerous" function according to you. If you or musl devs don't want
to provide strndupa(), the patch to be accepted in kmod is the one
that checks if strndupa() is available and add it to missing.h
otherwise. Why missing.h? To remember me to check it some time in
future to see if I can already remove some stuff.

actually i didn't even ask yet if we can ask strndupa, because it was first
encountered a week back in said eudev code, and my patch got immediately
merged.
when i see non-portable code, i fix the nonportable code, not libc, and send
a patch to upstream in the hope that it gets merged. if they don't merge it,
ok - yet another fork caused by maintainer stubborness.

Again, getting personal is not helping your cause.

Post by John Spencer
how do you like "kemod" as a name for the fork ?

And neither threatening like this

Post by John Spencer
yes, alloca() was added as it is widely used - you can't even build gcc
without it. the same can't be said about strndupa which is only used in one
mkdir code snippet copy-pasted from udev into a handful of projects.

you should grep more code.

Post by John Spencer
besides, afaik strndupa can't even be implemented as a function, since it
would return a pointer to automatic storage that's already out of scope (at
least not without using asm).

See reply above.

Post by John Spencer
that said, i am not sending you a macro replacement for a missing strndupa
and doing a full stop now. if you care about the musl userbase, merge this
patch, otherwise ignore it and live with a fork.

Post by Lucas De Marchi
Lucas De Marchi

P.S. i don't even use kmod myself (contrary to what poettering's twin
sievers believes), i ported this quickly for usage in dragora linux which is

Enough of that. Please refrain to insult people on this mailing list.


Lucas De Marchi