Re: [PATCH iproute2-next 1/9] dcb: app: expose dcb-app functions in new header

[Petr Machata <petrm@nvidia.com>]

Daniel Machon <daniel.machon@microchip.com> writes:

> Add new headerfile dcb-app.h that exposes the functions required later
> by dcb-rewr. The new dcb-rewr implementation will reuse much of the
> existing dcb-app code.
>
> I thought this called for a separate header file, instead of polluting
> the existing dcb.h file.
>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
>  dcb/dcb.h     |  9 ++-------
>  dcb/dcb_app.c | 54 ++++++++++++++++++------------------------------------
>  dcb/dcb_app.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+), 43 deletions(-)
>
> diff --git a/dcb/dcb.h b/dcb/dcb.h
> index d40664f29dad..4c8a4aa25e0c 100644
> --- a/dcb/dcb.h
> +++ b/dcb/dcb.h
> @@ -6,6 +6,8 @@
>  #include <stdbool.h>
>  #include <stddef.h>
>  
> +#include "dcb_app.h"
> +
>  /* dcb.c */
>  
>  struct dcb {
> @@ -54,13 +56,6 @@ void dcb_print_array_on_off(const __u8 *array, size_t size);
>  void dcb_print_array_kw(const __u8 *array, size_t array_size,
>  			const char *const kw[], size_t kw_size);
>  
> -/* dcb_app.c */
> -
> -int dcb_cmd_app(struct dcb *dcb, int argc, char **argv);
> -enum ieee_attrs_app dcb_app_attr_type_get(__u8 selector);
> -bool dcb_app_attr_type_validate(enum ieee_attrs_app type);
> -bool dcb_app_selector_validate(enum ieee_attrs_app type, __u8 selector);
> -

Why the move to a dedicated header? dcb.h ends up being the only client
and everybody consumes the prototypes through that file anyway. I don't
fine it necessary.

>  /* dcb_apptrust.c */
>  
>  int dcb_cmd_apptrust(struct dcb *dcb, int argc, char **argv);
> diff --git a/dcb/dcb_app.c b/dcb/dcb_app.c
> index eeb78e70f63f..df339babd8e6 100644
> --- a/dcb/dcb_app.c
> +++ b/dcb/dcb_app.c
> @@ -10,8 +10,6 @@
>  #include "utils.h"
>  #include "rt_names.h"
>  
> -#define DCB_APP_PCP_MAX 15
> -
>  static const char *const pcp_names[DCB_APP_PCP_MAX + 1] = {
>  	"0nd", "1nd", "2nd", "3nd", "4nd", "5nd", "6nd", "7nd",
>  	"0de", "1de", "2de", "3de", "4de", "5de", "6de", "7de"
> @@ -22,6 +20,7 @@ static const char *const ieee_attrs_app_names[__DCB_ATTR_IEEE_APP_MAX] = {
>  	[DCB_ATTR_DCB_APP] = "DCB_ATTR_DCB_APP"
>  };
>  
> +

This looks like a leftover.

>  static void dcb_app_help_add(void)
>  {
>  	fprintf(stderr,
> @@ -68,11 +67,6 @@ static void dcb_app_help(void)
>  	dcb_app_help_add();
>  }
>  
> -struct dcb_app_table {
> -	struct dcb_app *apps;
> -	size_t n_apps;
> -};
> -
>  enum ieee_attrs_app dcb_app_attr_type_get(__u8 selector)
>  {
>  	switch (selector) {
> @@ -105,7 +99,7 @@ bool dcb_app_selector_validate(enum ieee_attrs_app type, __u8 selector)
>  	return dcb_app_attr_type_get(selector) == type;
>  }
>  
> -static void dcb_app_table_fini(struct dcb_app_table *tab)
> +void dcb_app_table_fini(struct dcb_app_table *tab)
>  {
>  	free(tab->apps);
>  }
> @@ -124,8 +118,8 @@ static int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app)
>  	return 0;
>  }
>  
> -static void dcb_app_table_remove_existing(struct dcb_app_table *a,
> -					  const struct dcb_app_table *b)
> +void dcb_app_table_remove_existing(struct dcb_app_table *a,
> +				   const struct dcb_app_table *b)
>  {
>  	size_t ia, ja;
>  	size_t ib;
> @@ -152,8 +146,8 @@ static void dcb_app_table_remove_existing(struct dcb_app_table *a,
>  	a->n_apps = ja;
>  }
>  
> -static void dcb_app_table_remove_replaced(struct dcb_app_table *a,
> -					  const struct dcb_app_table *b)
> +void dcb_app_table_remove_replaced(struct dcb_app_table *a,
> +				   const struct dcb_app_table *b)
>  {
>  	size_t ia, ja;
>  	size_t ib;
> @@ -189,8 +183,7 @@ static void dcb_app_table_remove_replaced(struct dcb_app_table *a,
>  	a->n_apps = ja;
>  }
>  
> -static int dcb_app_table_copy(struct dcb_app_table *a,
> -			      const struct dcb_app_table *b)
> +int dcb_app_table_copy(struct dcb_app_table *a, const struct dcb_app_table *b)
>  {
>  	size_t i;
>  	int ret;
> @@ -217,18 +210,12 @@ static int dcb_app_cmp_cb(const void *a, const void *b)
>  	return dcb_app_cmp(a, b);
>  }
>  
> -static void dcb_app_table_sort(struct dcb_app_table *tab)
> +void dcb_app_table_sort(struct dcb_app_table *tab)
>  {
>  	qsort(tab->apps, tab->n_apps, sizeof(*tab->apps), dcb_app_cmp_cb);
>  }
>  
> -struct dcb_app_parse_mapping {
> -	__u8 selector;
> -	struct dcb_app_table *tab;
> -	int err;
> -};
> -
> -static void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data)
> +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data)
>  {
>  	struct dcb_app_parse_mapping *pm = data;
>  	struct dcb_app app = {
> @@ -260,7 +247,7 @@ static int dcb_app_parse_mapping_ethtype_prio(__u32 key, char *value, void *data
>  				 dcb_app_parse_mapping_cb, data);
>  }
>  
> -static int dcb_app_parse_pcp(__u32 *key, const char *arg)
> +int dcb_app_parse_pcp(__u32 *key, const char *arg)
>  {
>  	int i;
>  
> @@ -286,7 +273,7 @@ static int dcb_app_parse_mapping_pcp_prio(__u32 key, char *value, void *data)
>  				 dcb_app_parse_mapping_cb, data);
>  }
>  
> -static int dcb_app_parse_dscp(__u32 *key, const char *arg)
> +int dcb_app_parse_dscp(__u32 *key, const char *arg)
>  {
>  	if (parse_mapping_num_all(key, arg) == 0)
>  		return 0;
> @@ -377,12 +364,12 @@ static bool dcb_app_is_default(const struct dcb_app *app)
>  	       app->protocol == 0;
>  }
>  
> -static bool dcb_app_is_dscp(const struct dcb_app *app)
> +bool dcb_app_is_dscp(const struct dcb_app *app)
>  {
>  	return app->selector == IEEE_8021QAZ_APP_SEL_DSCP;
>  }
>  
> -static bool dcb_app_is_pcp(const struct dcb_app *app)
> +bool dcb_app_is_pcp(const struct dcb_app *app)
>  {
>  	return app->selector == DCB_APP_SEL_PCP;
>  }
> @@ -402,7 +389,7 @@ static bool dcb_app_is_port(const struct dcb_app *app)
>  	return app->selector == IEEE_8021QAZ_APP_SEL_ANY;
>  }
>  
> -static int dcb_app_print_key_dec(__u16 protocol)
> +int dcb_app_print_key_dec(__u16 protocol)
>  {
>  	return print_uint(PRINT_ANY, NULL, "%d:", protocol);
>  }
> @@ -412,7 +399,7 @@ static int dcb_app_print_key_hex(__u16 protocol)
>  	return print_uint(PRINT_ANY, NULL, "%x:", protocol);
>  }
>  
> -static int dcb_app_print_key_dscp(__u16 protocol)
> +int dcb_app_print_key_dscp(__u16 protocol)
>  {
>  	const char *name = rtnl_dsfield_get_name(protocol << 2);
>  
> @@ -422,7 +409,7 @@ static int dcb_app_print_key_dscp(__u16 protocol)
>  	return print_uint(PRINT_ANY, NULL, "%d:", protocol);
>  }
>  
> -static int dcb_app_print_key_pcp(__u16 protocol)
> +int dcb_app_print_key_pcp(__u16 protocol)
>  {
>  	/* Print in numerical form, if protocol value is out-of-range */
>  	if (protocol > DCB_APP_PCP_MAX)
> @@ -577,7 +564,7 @@ static int dcb_app_get_table_attr_cb(const struct nlattr *attr, void *data)
>  	return MNL_CB_OK;
>  }
>  
> -static int dcb_app_get(struct dcb *dcb, const char *dev, struct dcb_app_table *tab)
> +int dcb_app_get(struct dcb *dcb, const char *dev, struct dcb_app_table *tab)
>  {
>  	uint16_t payload_len;
>  	void *payload;
> @@ -594,11 +581,6 @@ static int dcb_app_get(struct dcb *dcb, const char *dev, struct dcb_app_table *t
>  	return 0;
>  }
>  
> -struct dcb_app_add_del {
> -	const struct dcb_app_table *tab;
> -	bool (*filter)(const struct dcb_app *app);
> -};
> -

This structure is a protocol between dcb_app_add_del() and
dcb_app_add_del_cb(). I don't think your patchset uses it elsewhere, so
it can be kept private.

>  static int dcb_app_add_del_cb(struct dcb *dcb, struct nlmsghdr *nlh, void *data)
>  {
>  	struct dcb_app_add_del *add_del = data;
> @@ -620,7 +602,7 @@ static int dcb_app_add_del_cb(struct dcb *dcb, struct nlmsghdr *nlh, void *data)
>  	return 0;
>  }
>  
> -static int dcb_app_add_del(struct dcb *dcb, const char *dev, int command,
> +int dcb_app_add_del(struct dcb *dcb, const char *dev, int command,
>  			   const struct dcb_app_table *tab,
>  			   bool (*filter)(const struct dcb_app *))

This has wrong indentation.

>  {
> diff --git a/dcb/dcb_app.h b/dcb/dcb_app.h
> new file mode 100644
> index 000000000000..8e7b010dcf75
> --- /dev/null
> +++ b/dcb/dcb_app.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __DCB_APP_H_
> +#define __DCB_APP_H_
> +
> +struct dcb;
> +
> +struct dcb_app_table {
> +	struct dcb_app *apps;
> +	size_t n_apps;
> +};
> +
> +struct dcb_app_add_del {
> +	const struct dcb_app_table *tab;
> +	bool (*filter)(const struct dcb_app *app);
> +};
> +
> +struct dcb_app_parse_mapping {
> +	__u8 selector;
> +	struct dcb_app_table *tab;
> +	int err;
> +};
> +
> +#define DCB_APP_PCP_MAX 15
> +
> +int dcb_cmd_app(struct dcb *dcb, int argc, char **argv);
> +
> +int dcb_app_get(struct dcb *dcb, const char *dev, struct dcb_app_table *tab);
> +int dcb_app_add_del(struct dcb *dcb, const char *dev, int command,
> +		    const struct dcb_app_table *tab,
> +		    bool (*filter)(const struct dcb_app *));
> +
> +void dcb_app_table_sort(struct dcb_app_table *tab);
> +void dcb_app_table_fini(struct dcb_app_table *tab);
> +int dcb_app_table_copy(struct dcb_app_table *a, const struct dcb_app_table *b);
> +void dcb_app_table_remove_replaced(struct dcb_app_table *a,
> +				   const struct dcb_app_table *b);
> +void dcb_app_table_remove_existing(struct dcb_app_table *a,
> +				   const struct dcb_app_table *b);
> +
> +bool dcb_app_is_pcp(const struct dcb_app *app);
> +bool dcb_app_is_dscp(const struct dcb_app *app);
> +
> +int dcb_app_print_key_dec(__u16 protocol);
> +int dcb_app_print_key_dscp(__u16 protocol);
> +int dcb_app_print_key_pcp(__u16 protocol);
> +
> +int dcb_app_parse_pcp(__u32 *key, const char *arg);
> +int dcb_app_parse_dscp(__u32 *key, const char *arg);
> +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data);
> +
> +bool dcb_app_selector_validate(enum ieee_attrs_app type, __u8 selector);
> +bool dcb_app_attr_type_validate(enum ieee_attrs_app type);
> +enum ieee_attrs_app dcb_app_attr_type_get(__u8 selector);
> +
> +#endif