Re: [PATCH -v2 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Ingo Molnar]

* Borislav Petkov <bp@xxxxxxxxx> wrote:

> On Fri, Jul 01, 2016 at 11:04:13AM +0200, Ingo Molnar wrote:
> 
> ...
> 
> > So the most robust way to define such bitfields is via a pattern like this:
> > 
> > enum devkmsg_log_bits {
> > 	__DEVKMSG_LOG_BIT_ON,
> > 	__DEVKMSG_LOG_BIT_OFF,
> > 	__DEVKMSG_LOG_BIT_LOCK,
> > };
> > 
> > enum devkmsg_log_masks {
> > 	DEVKMSG_LOG_MASK_ON		= BIT(__DEVKMSG_LOG_BIT_ON),
> > 	DEVKMSG_LOG_MASK_OFF		= BIT(__DEVKMSG_LOG_BIT_OFF),
> > 	DEVKMSG_LOG_MASK_LOCK		= BIT(__DEVKMSG_LOG_BIT_LOCK),
> 
> Agreed with so far, I'd only drop the "_MASK" thing and make it even
> easier on the eyes:
> 
> enum devkmsg_log_state {
> 	DEVKMSG_LOG_ON		= BIT(__DEVKMSG_LOG_BIT_ON),
> 	DEVKMSG_LOG_OFF		= BIT(__DEVKMSG_LOG_BIT_OFF),
> 	DEVKMSG_LOCK		= BIT(__DEVKMSG_LOG_BIT_LOCK),
> };

It's just a nit, but generally it's nice to know the character of such values - 
i.e. in case that it's a bit mask that has to be used with bit ops. That's more 
important IMHO than brevity. This means that possibly buggy code like this stands 
out immediately:

	if (devkmgs_log == DEVKMSG_LOG_MASK_ON)

while this one:

	if (devkmgs_log == DEVKMSG_LOG_ON)

might slip through.

But no strong feelings either way!

Thanks,

	Ingo