On Tue, Dec 24, 2019 at 11:54:58AM +0200, Topi Miettinen wrote:
On 24.12.2019 4.54, Lucas De Marchi wrote:
On Mon, Dec 23, 2019 at 9:07 AM Topi Miettinen <toiwoton@xxxxxxxxx> wrote:
Don't use exit status of a command directly as errno code, callers
will be confused.
Signed-off-by: Topi Miettinen <toiwoton@xxxxxxxxx>
---
libkmod/libkmod-module.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 8044a8f..6031d80 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -983,11 +983,13 @@ static int command_do(struct kmod_module *mod,
const char *type,
if (err == -1 || WEXITSTATUS(err)) {
ERR(mod->ctx, "Error running %s command for %s\n",
type,
modname);
- if (err != -1)
- err = -WEXITSTATUS(err);
I don't think we actually care about differentiating them. So just a plain
return -EINVAL; here would suffice, makes sense?
I think it would lose potentially valuable information. For example
EPERM could tell the system administrator of a problem with MAC
configuration preventing execution of the shell, ENOENT could show
that the shell or shared libraries are missing and so forth.
makes sense, but we take decisions on the callers depending on the
return value. I don't want to mix that with return values from the
commands executed. E.g. if the command returned EEXIST the caller would
treat a fail differently.
I think it would be good here to give different error messages and
always return -EINVAL. I'm thinking on squashing the following diff,
what do you think?
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 6031d80..714ee21 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -980,13 +980,16 @@ static int command_do(struct kmod_module *mod, const char *type,
err = system(cmd);
unsetenv("MODPROBE_MODULE");
- if (err == -1 || WEXITSTATUS(err)) {
- ERR(mod->ctx, "Error running %s command for %s\n",
- type, modname);
- if (err != -1) /* nonzero exit status: something bad happened */
- return -EINVAL;
- else /* child process could not be created */
- return -errno;
+ if (err == -1) {
+ ERR(mod->ctx, "Could not run %s command '%s' for module %s: %m\n",
+ type, cmd, modname);
+ return -EINVAL;
+ }
+
+ if (WEXITSTATUS(err)) {
+ ERR(mod->ctx, "Error running %s command '%s' for module %s: retcode %d\n",
+ type, cmd, modname, WEXITSTATUS(err));
+ return -EINVAL;
}
return 0;
thanks
Lucas De Marchi
