Re: [PATCH v1 05/10] ipmi_si: Introduce panic_event_str array

[Corey Minyard <minyard@acm.org>]

On Tue, Mar 30, 2021 at 09:16:44PM +0300, Andy Shevchenko wrote:
> Instead of twice repeat the constant literals, introduce
> panic_event_str array. It allows to simplify the code with
> help of match_string() API.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c | 49 ++++++++++-------------------
>  1 file changed, 17 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index f19f0f967e28..c7d37366d7bb 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -52,8 +52,12 @@ static bool drvregistered;
>  enum ipmi_panic_event_op {
>  	IPMI_SEND_PANIC_EVENT_NONE,
>  	IPMI_SEND_PANIC_EVENT,
> -	IPMI_SEND_PANIC_EVENT_STRING
> +	IPMI_SEND_PANIC_EVENT_STRING,
> +	IPMI_SEND_PANIC_EVENT_MAX
>  };

This is a nice change.  Can you add a comment here so that readers know
that the above enum and the following array are tied numerically?

-corey

> +
> +static const char *const panic_event_str[] = { "none", "event", "string", NULL };
> +
>  #ifdef CONFIG_IPMI_PANIC_STRING
>  #define IPMI_PANIC_DEFAULT IPMI_SEND_PANIC_EVENT_STRING
>  #elif defined(CONFIG_IPMI_PANIC_EVENT)
> @@ -68,46 +72,27 @@ static int panic_op_write_handler(const char *val,
>  				  const struct kernel_param *kp)
>  {
>  	char valcp[16];
> -	char *s;
> -
> -	strncpy(valcp, val, 15);
> -	valcp[15] = '\0';
> +	int e;
>  
> -	s = strstrip(valcp);
> -
> -	if (strcmp(s, "none") == 0)
> -		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT_NONE;
> -	else if (strcmp(s, "event") == 0)
> -		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT;
> -	else if (strcmp(s, "string") == 0)
> -		ipmi_send_panic_event = IPMI_SEND_PANIC_EVENT_STRING;
> -	else
> -		return -EINVAL;
> +	strscpy(valcp, val, sizeof(valcp));
> +	e = match_string(panic_event_str, -1, strstrip(valcp));
> +	if (e < 0)
> +		return e;
>  
> +	ipmi_send_panic_event = e;
>  	return 0;
>  }
>  
>  static int panic_op_read_handler(char *buffer, const struct kernel_param *kp)
>  {
> -	switch (ipmi_send_panic_event) {
> -	case IPMI_SEND_PANIC_EVENT_NONE:
> -		strcpy(buffer, "none\n");
> -		break;
> -
> -	case IPMI_SEND_PANIC_EVENT:
> -		strcpy(buffer, "event\n");
> -		break;
> -
> -	case IPMI_SEND_PANIC_EVENT_STRING:
> -		strcpy(buffer, "string\n");
> -		break;
> +	const char *event_str;
>  
> -	default:
> -		strcpy(buffer, "???\n");
> -		break;
> -	}
> +	if (ipmi_send_panic_event >= IPMI_SEND_PANIC_EVENT_MAX)
> +		event_str = "???";
> +	else
> +		event_str = panic_event_str[ipmi_send_panic_event];
>  
> -	return strlen(buffer);
> +	return sprintf(buffer, "%s\n", event_str);
>  }
>  
>  static const struct kernel_param_ops panic_op_ops = {
> -- 
> 2.30.2
>