Re: [PATCH v2 1/2] printk: Introduce FORCE_CON flag

[Marcos Paulo de Souza]

On Tue, 2024-11-05 at 22:40 +0106, John Ogness wrote:
> On 2024-11-05, Marcos Paulo de Souza <mpdesouza@xxxxxxxx> wrote:
> > @@ -2947,6 +2953,7 @@ bool printk_get_next_message(struct
> > printk_message *pmsg, u64 seq,
> >  	struct printk_info info;
> >  	struct printk_record r;
> >  	size_t len = 0;
> > +	bool force_con;
> >  
> >  	/*
> >  	 * Formatting extended messages requires a separate
> > buffer, so use the
> > @@ -2965,9 +2972,13 @@ bool printk_get_next_message(struct
> > printk_message *pmsg, u64 seq,
> >  
> >  	pmsg->seq = r.info->seq;
> >  	pmsg->dropped = r.info->seq - seq;
> > +	force_con = r.info->flags & LOG_FORCE_CON;
> >  
> > -	/* Skip record that has level above the console loglevel.
> > */
> > -	if (may_suppress && suppress_message_printing(r.info-
> > >level))
> > +	/*
> > +	 * Skip records that are not forced to be printed on
> > consoles and that
> > +	 * has level above the console loglevel.
> > +	 */
> > +	if (!force_con && may_suppress &&
> > suppress_message_printing(r.info->level))
> >  		goto out;
> 
> Rather than adding a new local variable, setting it, and expanding
> the
> condition, it might be cleaner to just update @may_suppress before
> the
> condition check?
> 
> 	/* Records forced to be printed on consoles must not be
> skipped. */
> 	may_suppress &= !(r.info->flags & LOG_FORCE_CON);

Well, your suggestion seems clever than what I did :)

IHMO, I would prefer the new variable as it's easier to read (for me at
least), but I can change if you think it's better..

> 
> Feel free to ignore this suggestion if you think having an extra
> variable is easier to follow.
> 
> With or without suggested change:
> 
> Reviewed-by: John Ogness <john.ogness@xxxxxxxxxxxxx>

Thanks John!