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 <[email protected]>
> > ---
> > 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
