| Message ID | f8c64aed-b4a1-e43f-ed4b-f99236932477@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libkmod-module: convert return value from system() to errno | expand |
On Mon, Dec 23, 2019 at 9:07 AM Topi Miettinen <toiwoton@gmail.com> wrote: > > Don't use exit status of a command directly as errno code, callers > will be confused. > > Signed-off-by: Topi Miettinen <toiwoton@gmail.com> > --- > 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? Lucas De Marchi > + if (err != -1) /* nonzero exit status: something bad > happened */ > + return -EINVAL; > + else /* child process could not be created */ > + return -errno; > } > > - return err; > + return 0; > } > > struct probe_insert_cb { > -- > 2.24.0
On 24.12.2019 4.54, Lucas De Marchi wrote: > On Mon, Dec 23, 2019 at 9:07 AM Topi Miettinen <toiwoton@gmail.com> wrote: >> >> Don't use exit status of a command directly as errno code, callers >> will be confused. >> >> Signed-off-by: Topi Miettinen <toiwoton@gmail.com> >> --- >> 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. -Topi
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@gmail.com> wrote: >>> >>>Don't use exit status of a command directly as errno code, callers >>>will be confused. >>> >>>Signed-off-by: Topi Miettinen <toiwoton@gmail.com> >>>--- >>> 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
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@gmail.com> >>> wrote: >>>> >>>> Don't use exit status of a command directly as errno code, callers >>>> will be confused. >>>> >>>> Signed-off-by: Topi Miettinen <toiwoton@gmail.com> >>>> --- >>>> 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
From f560864a4a8fd61c2881cfefb970db27c2418690 Mon Sep 17 00:00:00 2001 From: Topi Miettinen <toiwoton@gmail.com> Date: Mon, 23 Dec 2019 18:58:13 +0200 Subject: [PATCH] libkmod-module: convert return value from system() to errno Don't use exit status of a command directly as errno code, callers will be confused. Signed-off-by: Topi Miettinen <toiwoton@gmail.com> --- 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); + if (err != -1) /* nonzero exit status: something bad happened */ + return -EINVAL; + else /* child process could not be created */ + return -errno; } - return err; + return 0; } struct probe_insert_cb { -- 2.24.0
Don't use exit status of a command directly as errno code, callers will be confused. Signed-off-by: Topi Miettinen <toiwoton@gmail.com> --- libkmod/libkmod-module.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) } struct probe_insert_cb {