Re: [patch iproute2/net-next repost] devlink: Add support for pipeline debug (dpipe)

[Jiri Pirko <jiri@resnulli.us>]

Thu, Apr 13, 2017 at 01:45:25AM CEST, stephen@networkplumber.org wrote:
>On Tue, 28 Mar 2017 17:26:54 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>>  
>>  #define pr_err(args...) fprintf(stderr, ##args)
>> -#define pr_out(args...) fprintf(stdout, ##args)
>> +#define pr_out(args...)						\
>> +	do {							\
>> +		if (g_indent_newline) {				\
>> +			fprintf(stdout, "%s", g_indent_str);	\
>> +			g_indent_newline = false;		\
>> +		}						\
>> +		fprintf(stdout, ##args);			\
>> +	} while (0)
>> +
>>  #define pr_out_sp(num, args...)					\
>>  	do {							\
>>  		int ret = fprintf(stdout, ##args);		\
>> @@ -42,6 +50,35 @@
>>  			fprintf(stdout, "%*s", num - ret, "");	\
>>  	} while (0)
>>  
>> +static int g_indent_level;
>> +static bool g_indent_newline;
>> +#define INDENT_STR_STEP 2
>> +#define INDENT_STR_MAXLEN 32
>> +static char g_indent_str[INDENT_STR_MAXLEN + 1] = "";
>> +
>> +static void __pr_out_indent_inc(void)
>> +{
>> +	if (g_indent_level + INDENT_STR_STEP > INDENT_STR_MAXLEN)
>> +		return;
>> +	g_indent_level += INDENT_STR_STEP;
>> +	memset(g_indent_str, ' ', sizeof(g_indent_str));
>> +	g_indent_str[g_indent_level] = '\0';
>> +}
>> +
>> +static void __pr_out_indent_dec(void)
>> +{
>> +	if (g_indent_level - INDENT_STR_STEP < 0)
>> +		return;
>> +	g_indent_level -= INDENT_STR_STEP;
>> +	g_indent_str[g_indent_level] = '\0';
>> +}
>> +
>> +static void __pr_out_newline(void)
>> +{
>> +	pr_out("\n");
>> +	g_indent_newline = true;
>> +}
>> +
>
>Thanks for adding the support. Like many reviews, I am fine with the
>functionality but it is the details of the implementation that are of
>concern.
>
>Why this new set of output formatting routines, this doesn't resemble other code
>in ip utilities. Looks more like you copied and pasted it from somewhere else.
>The indentation in existing ip command output isn't pretty or fancy but it works.
>
>Please try and make this code look like all the other code. Yes, I know it
>may not be to your taste (it isn't mine either), but consistency is more important
>than individual style.

We actually took this code from teamdctl (at least it was an influence).
Devlink style is so much different in every aspect from the rest of the
iproute2 suite. And I did it on purpose, because it is much nicer and
easier to read. I would like to continue on this and don't do things in
the was the existing tools do. I don't see any problem with that.

	
>
>> @ -314,6 +356,102 @@ static int attr_cb(const struct nlattr *attr, void *data)
>>  	if (type == DEVLINK_ATTR_ESWITCH_INLINE_MODE &&
>>  	    mnl_attr_validate(attr, MNL_TYPE_U8) < 0)
>>  		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_TABLES &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_TABLE &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_TABLE_NAME &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_STRING) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_TABLE_SIZE &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_TABLE_MATCHES &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_TABLE_ACTIONS &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_TABLE_COUNTERS_ENABLED &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U8) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ENTRIES &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ENTRY &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ENTRY_INDEX &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ENTRY_MATCH_VALUES &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ENTRY_ACTION_VALUES &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ENTRY_COUNTER &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U64) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_MATCH &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_MATCH_VALUE &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_MATCH_TYPE &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ACTION &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ACTION_VALUE &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_ACTION_TYPE &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_VALUE_MAPPING &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_HEADERS &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_HEADER &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_HEADER_NAME &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_STRING) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_HEADER_ID &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_HEADER_FIELDS &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_HEADER_GLOBAL &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U8) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_HEADER_INDEX &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_FIELD &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_FIELD_NAME &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_STRING) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_FIELD_ID &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_FIELD_BITWIDTH &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
>> +		return MNL_CB_ERROR;
>> +	if (type == DEVLINK_ATTR_DPIPE_FIELD_MAPPING_TYPE &&
>> +	    mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
>> +		return MNL_CB_ERROR;
>>  	tb[type] = attr;
>>  	return MNL_CB_OK;
>>  }
>
>Ok, this code has gotten so long that it really needs to be handled as a table
>rather than linear sequence of if's.
>
>