| Message ID | VI1PR02MB48291E3BDB7E764ED67154949BA42@VI1PR02MB4829.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [kmod] tools/rmmod: fix garbled error message | expand |
| Context | Check | Description |
|---|---|---|
| mcgrof/vmtest-modules-next-PR | fail | merge-conflict |
On Mon, Mar 24, 2025 at 06:35:53PM +0100, Yannick Le Pennec wrote: >a6f9cd0 ("tools/rmmod: consistently use ERR logging facility") fixed >the split between syslog and stderr of various error message substrings >by calling the ERR macro instead of writing directly to stderr, but in >doing so also completely mangled the output because the ERR macro >decorates its arguments: > > $ rmmod iwlwifi > rmmod: ERROR: Module iwlwifi is in use by:rmmod: ERROR: iwlmvmrmmod: ERROR: > >And in syslog: > > $ rmmod -s iwlwifi > 2025-03-24T17:22:34.878318+01:00 mangolassi rmmod: ERROR: Module iwlwifi is in use by: > 2025-03-24T17:22:34.889145+01:00 mangolassi rmmod: ERROR: iwlmvm > 2025-03-24T17:22:34.889224+01:00 mangolassi rmmod: ERROR: > >This commit fixes that by building the holder names list with a strbuf >and then passes the whole thing at once to ERR. doesn't that mean the syslog version will only have 1 ERROR now? anyway, queued for CI at https://github.com/kmod-project/kmod/pull/328 thanks Lucas De Marchi > >Fixes: a6f9cd0 ("tools/rmmod: consistently use ERR logging facility") >Signed-off-by: Yannick Le Pennec <yannick.lepennec@live.fr> >--- > tools/rmmod.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > >diff --git a/tools/rmmod.c b/tools/rmmod.c >index 962d850..61f2e00 100644 >--- a/tools/rmmod.c >+++ b/tools/rmmod.c >@@ -15,6 +15,7 @@ > #include <sys/types.h> > > #include <shared/macro.h> >+#include <shared/strbuf.h> > > #include <libkmod/libkmod.h> > >@@ -63,16 +64,18 @@ static int check_module_inuse(struct kmod_module *mod) > > holders = kmod_module_get_holders(mod); > if (holders != NULL) { >+ DECLARE_STRBUF_WITH_STACK(buf, 128); > struct kmod_list *itr; > >- ERR("Module %s is in use by:", kmod_module_get_name(mod)); >- > kmod_list_foreach(itr, holders) { > struct kmod_module *hm = kmod_module_get_module(itr); >- ERR(" %s", kmod_module_get_name(hm)); >+ strbuf_pushchar(&buf, ' '); >+ strbuf_pushchars(&buf, kmod_module_get_name(hm)); > kmod_module_unref(hm); > } >- ERR("\n"); >+ >+ ERR("Module %s is in use by:%s\n", kmod_module_get_name(mod), >+ strbuf_str(&buf)); > > kmod_module_unref_list(holders); > return -EBUSY; >-- >2.49.0
On Thu, 2025-03-27 at 23:13 -0500, Lucas De Marchi wrote: > On Mon, Mar 24, 2025 at 06:35:53PM +0100, Yannick Le Pennec wrote: > > a6f9cd0 ("tools/rmmod: consistently use ERR logging facility") fixed > > the split between syslog and stderr of various error message substrings > > by calling the ERR macro instead of writing directly to stderr, but in > > doing so also completely mangled the output because the ERR macro > > decorates its arguments: > > > > $ rmmod iwlwifi > > rmmod: ERROR: Module iwlwifi is in use by:rmmod: ERROR: iwlmvmrmmod: ERROR: > > > > And in syslog: > > > > $ rmmod -s iwlwifi > > 2025-03-24T17:22:34.878318+01:00 mangolassi rmmod: ERROR: Module iwlwifi is in use by: > > 2025-03-24T17:22:34.889145+01:00 mangolassi rmmod: ERROR: iwlmvm > > 2025-03-24T17:22:34.889224+01:00 mangolassi rmmod: ERROR: > > > > This commit fixes that by building the holder names list with a strbuf > > and then passes the whole thing at once to ERR. > > doesn't that mean the syslog version will only have 1 ERROR now? Yes indeed but I think that's the right behaviour. Prior to a6f9cd0 there was only 1 error going to syslog (because the rest of the line was sent to stderr erroneously). With a6f9cd0 N + 2 (with N = number of holders) error lines go to syslog, which I don't think is what was intended? After all the stderr message itself was always one line, and furthermore the lone ERR("\n") was odd. > > anyway, queued for CI at https://github.com/kmod-project/kmod/pull/328 > > thanks > Lucas De Marchi > > > > > Fixes: a6f9cd0 ("tools/rmmod: consistently use ERR logging facility") > > Signed-off-by: Yannick Le Pennec <yannick.lepennec@live.fr> > > --- > > tools/rmmod.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/tools/rmmod.c b/tools/rmmod.c > > index 962d850..61f2e00 100644 > > --- a/tools/rmmod.c > > +++ b/tools/rmmod.c > > @@ -15,6 +15,7 @@ > > #include <sys/types.h> > > > > #include <shared/macro.h> > > +#include <shared/strbuf.h> > > > > #include <libkmod/libkmod.h> > > > > @@ -63,16 +64,18 @@ static int check_module_inuse(struct kmod_module *mod) > > > > holders = kmod_module_get_holders(mod); > > if (holders != NULL) { > > + DECLARE_STRBUF_WITH_STACK(buf, 128); > > struct kmod_list *itr; > > > > - ERR("Module %s is in use by:", kmod_module_get_name(mod)); > > - > > kmod_list_foreach(itr, holders) { > > struct kmod_module *hm = kmod_module_get_module(itr); > > - ERR(" %s", kmod_module_get_name(hm)); > > + strbuf_pushchar(&buf, ' '); > > + strbuf_pushchars(&buf, kmod_module_get_name(hm)); > > kmod_module_unref(hm); > > } > > - ERR("\n"); > > + > > + ERR("Module %s is in use by:%s\n", kmod_module_get_name(mod), > > + strbuf_str(&buf)); > > > > kmod_module_unref_list(holders); > > return -EBUSY; > > -- > > 2.49.0
On Fri, Mar 28, 2025 at 12:36:25PM +0100, Yannick Le Pennec wrote: >On Thu, 2025-03-27 at 23:13 -0500, Lucas De Marchi wrote: >> On Mon, Mar 24, 2025 at 06:35:53PM +0100, Yannick Le Pennec wrote: >> > a6f9cd0 ("tools/rmmod: consistently use ERR logging facility") fixed >> > the split between syslog and stderr of various error message substrings >> > by calling the ERR macro instead of writing directly to stderr, but in >> > doing so also completely mangled the output because the ERR macro >> > decorates its arguments: >> > >> > $ rmmod iwlwifi >> > rmmod: ERROR: Module iwlwifi is in use by:rmmod: ERROR: iwlmvmrmmod: ERROR: >> > >> > And in syslog: >> > >> > $ rmmod -s iwlwifi >> > 2025-03-24T17:22:34.878318+01:00 mangolassi rmmod: ERROR: Module iwlwifi is in use by: >> > 2025-03-24T17:22:34.889145+01:00 mangolassi rmmod: ERROR: iwlmvm >> > 2025-03-24T17:22:34.889224+01:00 mangolassi rmmod: ERROR: >> > >> > This commit fixes that by building the holder names list with a strbuf >> > and then passes the whole thing at once to ERR. >> >> doesn't that mean the syslog version will only have 1 ERROR now? > >Yes indeed but I think that's the right behaviour. Prior to a6f9cd0 there was >only 1 error going to syslog (because the rest of the line was sent to stderr >erroneously). With a6f9cd0 N + 2 (with N = number of holders) error lines go to >syslog, which I don't think is what was intended? After all the stderr message >itself was always one line, and furthermore the lone ERR("\n") was odd. oh... ok. I missed the fact that you dropped the newline and just added a space as sep. Applied, thanks. Lucas De Marchi > >> >> anyway, queued for CI at https://github.com/kmod-project/kmod/pull/328 >> >> thanks >> Lucas De Marchi >> >> > >> > Fixes: a6f9cd0 ("tools/rmmod: consistently use ERR logging facility") >> > Signed-off-by: Yannick Le Pennec <yannick.lepennec@live.fr> >> > --- >> > tools/rmmod.c | 11 +++++++---- >> > 1 file changed, 7 insertions(+), 4 deletions(-) >> > >> > diff --git a/tools/rmmod.c b/tools/rmmod.c >> > index 962d850..61f2e00 100644 >> > --- a/tools/rmmod.c >> > +++ b/tools/rmmod.c >> > @@ -15,6 +15,7 @@ >> > #include <sys/types.h> >> > >> > #include <shared/macro.h> >> > +#include <shared/strbuf.h> >> > >> > #include <libkmod/libkmod.h> >> > >> > @@ -63,16 +64,18 @@ static int check_module_inuse(struct kmod_module *mod) >> > >> > holders = kmod_module_get_holders(mod); >> > if (holders != NULL) { >> > + DECLARE_STRBUF_WITH_STACK(buf, 128); >> > struct kmod_list *itr; >> > >> > - ERR("Module %s is in use by:", kmod_module_get_name(mod)); >> > - >> > kmod_list_foreach(itr, holders) { >> > struct kmod_module *hm = kmod_module_get_module(itr); >> > - ERR(" %s", kmod_module_get_name(hm)); >> > + strbuf_pushchar(&buf, ' '); >> > + strbuf_pushchars(&buf, kmod_module_get_name(hm)); >> > kmod_module_unref(hm); >> > } >> > - ERR("\n"); >> > + >> > + ERR("Module %s is in use by:%s\n", kmod_module_get_name(mod), >> > + strbuf_str(&buf)); >> > >> > kmod_module_unref_list(holders); >> > return -EBUSY; >> > -- >> > 2.49.0
diff --git a/tools/rmmod.c b/tools/rmmod.c index 962d850..61f2e00 100644 --- a/tools/rmmod.c +++ b/tools/rmmod.c @@ -15,6 +15,7 @@ #include <sys/types.h> #include <shared/macro.h> +#include <shared/strbuf.h> #include <libkmod/libkmod.h> @@ -63,16 +64,18 @@ static int check_module_inuse(struct kmod_module *mod) holders = kmod_module_get_holders(mod); if (holders != NULL) { + DECLARE_STRBUF_WITH_STACK(buf, 128); struct kmod_list *itr; - ERR("Module %s is in use by:", kmod_module_get_name(mod)); - kmod_list_foreach(itr, holders) { struct kmod_module *hm = kmod_module_get_module(itr); - ERR(" %s", kmod_module_get_name(hm)); + strbuf_pushchar(&buf, ' '); + strbuf_pushchars(&buf, kmod_module_get_name(hm)); kmod_module_unref(hm); } - ERR("\n"); + + ERR("Module %s is in use by:%s\n", kmod_module_get_name(mod), + strbuf_str(&buf)); kmod_module_unref_list(holders); return -EBUSY;
a6f9cd0 ("tools/rmmod: consistently use ERR logging facility") fixed the split between syslog and stderr of various error message substrings by calling the ERR macro instead of writing directly to stderr, but in doing so also completely mangled the output because the ERR macro decorates its arguments: $ rmmod iwlwifi rmmod: ERROR: Module iwlwifi is in use by:rmmod: ERROR: iwlmvmrmmod: ERROR: And in syslog: $ rmmod -s iwlwifi 2025-03-24T17:22:34.878318+01:00 mangolassi rmmod: ERROR: Module iwlwifi is in use by: 2025-03-24T17:22:34.889145+01:00 mangolassi rmmod: ERROR: iwlmvm 2025-03-24T17:22:34.889224+01:00 mangolassi rmmod: ERROR: This commit fixes that by building the holder names list with a strbuf and then passes the whole thing at once to ERR. Fixes: a6f9cd0 ("tools/rmmod: consistently use ERR logging facility") Signed-off-by: Yannick Le Pennec <yannick.lepennec@live.fr> --- tools/rmmod.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)