Re: [PATCH 1/2] modpost: rework and consolidate logging interface

[Jessica Yu]

+++ Joe Perches [25/02/20 14:30 -0800]:
> On Tue, 2020-02-25 at 18:35 +0100, Jessica Yu wrote:
> > Rework modpost's logging interface by consolidating merror(), warn(),
> > and fatal() to use a single function, modpost_log(). Introduce different
> > logging levels (WARN, ERROR, FATAL) as well as a conditional warn
> > (warn_unless()). The conditional warn is useful in determining whether
> > to use merror() or warn() based on a condition. This reduces code
> > duplication overall.
> []
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> []
> > @@ -51,41 +51,39 @@ enum export {
> >
> >  #define MODULE_NAME_LEN (64 - sizeof(Elf_Addr))
> >
> > -#define PRINTF __attribute__ ((format (printf, 1, 2)))
> > +#define PRINTF __attribute__ ((format (printf, 2, 3)))
> >
> > -PRINTF void fatal(const char *fmt, ...)
> > +PRINTF void modpost_log(enum loglevel loglevel, const char *fmt, ...)
> >  {
> > +	char *level = NULL;
> >  	va_list arglist;
> >
> > -	fprintf(stderr, "FATAL: ");
> > +	switch(loglevel) {
> > +	case(LOG_WARN):
> > +		level = "WARNING: ";
> > +		break;
> > +	case(LOG_ERROR):
> > +		level = "ERROR: ";
> > +		break;
> > +	case(LOG_FATAL):
> > +		level = "FATAL: ";
> > +		break;
> > +	default: /* invalid loglevel, ignore */
> > +		break;
>
> Odd parentheses around case labels and
> likely level should be initialized as ""
> and not NULL.
>
> 	const char *level = "";
> 	...
> 	switch (loglevel) {
> 	case LOG_WARN:
> 		level = "WARNING: ";
> 		break;
> 	...
> 	}

Thanks for the review! Will fix this in v2 shortly.