Re: [PATCH for-next 1/4] devlink: refactor validation of finding required arguments

[Jiri Pirko <jiri@resnulli.us>]

Sun, Feb 10, 2019 at 07:28:46PM CET, ayal@mellanox.com wrote:
>Introducing argument's metadata structure matching a bitmap flag per
>required argument and an error message if missing. Using this static
>array to refactor validation of finding required arguments in devlink
>command line and to ease further maintenance.
>
>Signed-off-by: Aya Levin <ayal@mellanox.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> devlink/devlink.c | 155 +++++++++++++++++-------------------------------------
> 1 file changed, 47 insertions(+), 108 deletions(-)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index d823512a4030..a05755385a49 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -39,6 +39,7 @@
> #define PARAM_CMODE_RUNTIME_STR "runtime"
> #define PARAM_CMODE_DRIVERINIT_STR "driverinit"
> #define PARAM_CMODE_PERMANENT_STR "permanent"
>+#define DL_ARGS_REQUIRED_MAX_ERR_LEN 80
> 
> static int g_new_line_count;
> 
>@@ -950,6 +951,51 @@ static int param_cmode_get(const char *cmodestr,
> 	return 0;
> }
> 
>+struct dl_args_metadata {
>+	uint32_t o_flag;
>+	char err_msg[DL_ARGS_REQUIRED_MAX_ERR_LEN];
>+};
>+
>+static const struct dl_args_metadata dl_args_required[] = {
>+	{DL_OPT_PORT_TYPE,	      "Port type not set.\n"},
>+	{DL_OPT_PORT_COUNT,	      "Port split count option expected.\n"},
>+	{DL_OPT_SB_POOL,	      "Pool index option expected.\n"},
>+	{DL_OPT_SB_SIZE,	      "Pool size option expected.\n"},
>+	{DL_OPT_SB_TYPE,	      "Pool type option expected.\n"},
>+	{DL_OPT_SB_THTYPE,	      "Pool threshold type option expected.\n"},
>+	{DL_OPT_SB_TH,		      "Threshold option expected.\n"},
>+	{DL_OPT_SB_TC,		      "TC index option expected.\n"},
>+	{DL_OPT_ESWITCH_MODE,	      "E-Switch mode option expected.\n"},
>+	{DL_OPT_ESWITCH_INLINE_MODE,  "E-Switch inline-mode option expected.\n"},
>+	{DL_OPT_DPIPE_TABLE_NAME,     "Dpipe table name expected\n"},
>+	{DL_OPT_DPIPE_TABLE_COUNTERS, "Dpipe table counter state expected\n"},
>+	{DL_OPT_ESWITCH_ENCAP_MODE,   "E-Switch encapsulation option expected.\n"},
>+	{DL_OPT_PARAM_NAME,	      "Parameter name expected.\n"},
>+	{DL_OPT_PARAM_VALUE,	      "Value to set expected.\n"},
>+	{DL_OPT_PARAM_CMODE,	      "Configuration mode expected.\n"},
>+	{DL_OPT_REGION_SNAPSHOT_ID,   "Region snapshot id expected.\n"},
>+	{DL_OPT_REGION_ADDRESS,	      "Region address value expected.\n"},
>+	{DL_OPT_REGION_LENGTH,	      "Region length value expected.\n"},

Remove the "\n" from there and put it to the pr_err() call.


>+};
>+
>+static int validate_finding_required_dl_args(uint32_t o_required,

Please maintain the current naming scheme of functions. This should be
named something like:
dl_args_finding_required_validate()


>+					     uint32_t o_found)
>+{
>+	uint32_t dl_args_required_size;
>+	uint32_t o_flag;
>+	int i;
>+
>+	dl_args_required_size = ARRAY_SIZE(dl_args_required);
>+	for (i = 0; i < dl_args_required_size; i++) {
>+		o_flag = dl_args_required[i].o_flag;
>+		if ((o_required & o_flag) && !(o_found & o_flag)) {
>+			pr_err("%s", dl_args_required[i].err_msg);
>+			return -EINVAL;
>+		}
>+	}
>+	return 0;
>+}
>+

[...]