Re: [PATCH] libkmod-module: convert return value from system() to errno

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 29.12.2019 0.59, Lucas De Marchi wrote:
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?

OK, maybe that's better.

-Topi


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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux