Message-ID: <ZG2xBsorejY7v5l1@DEN-LT-70577>
Date: Wed, 24 May 2023 06:39:03 +0000
From: <Daniel.Machon@...rochip.com>
To: <petrm@...dia.com>
CC: <netdev@...r.kernel.org>, <dsahern@...nel.org>,
	<stephen@...workplumber.org>, <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH iproute2-next 1/9] dcb: app: expose dcb-app functions in
 new header

> > 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@...rochip.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.

I did try to rationalize that a bit in the commit description. I thought
the amount of exposed app functions ended up polutting the dcb header.
Maybe it is not that bad - can move them back in the next v.

> 
> >  /* 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.

Yep. You are right.

> 
> >  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.

Ouch. Will fix in next v.

> 
> >  {
> > 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
> 

Powered by blists - more mailing lists