| Message ID | 20230510-dcb-rewr-v1-3-83adc1f93356@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | Introduce new dcb-rewr subcommand | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, 22 May 2023 20:41:06 +0200 Daniel Machon <daniel.machon@microchip.com> wrote: > +int dcb_app_print_pid_dec(__u16 protocol) > { > - return print_uint(PRINT_ANY, NULL, "%d:", protocol); > + return print_uint(PRINT_ANY, NULL, "%d", protocol); > } Should be %u for unsigned value.
Daniel Machon <daniel.machon@microchip.com> writes: > -static void dcb_app_print_filtered(const struct dcb_app_table *tab, > - bool (*filter)(const struct dcb_app *), > - int (*print_key)(__u16 protocol), > - const char *json_name, > - const char *fp_name) > +void dcb_app_print_filtered(const struct dcb_app_table *tab, > + bool (*filter)(const struct dcb_app *), > + int (*print_pid)(__u16 protocol), > + const char *json_name, const char *fp_name) > { > bool first = true; > size_t i; > @@ -439,8 +437,14 @@ static void dcb_app_print_filtered(const struct dcb_app_table *tab, > } > > open_json_array(PRINT_JSON, NULL); > - print_key(app->protocol); > - print_uint(PRINT_ANY, NULL, "%d ", app->priority); > + if (tab->attr == DCB_ATTR_IEEE_APP_TABLE) { > + print_pid(app->protocol); > + print_uint(PRINT_ANY, NULL, ":%d", app->priority); > + } else { > + print_uint(PRINT_ANY, NULL, "%d:", app->priority); > + print_pid(app->protocol); > + } I really dislike the attribute dispatch. I feels too much like mixing abstraction layers. I think the callback should take a full struct dcb_app pointer and format it as appropriate. Then you can model the rewrite table differently from the app table by providing a callback that invokes the print_ helpers in the correct order. The app->protocol field as such is not really necessary IMHO, because the function that invokes the helpers understands what kind of table it is dealing with and could provide it as a parameter. But OK, I guess it makes sense and probably saves some boilerplate parameterization. > + print_string(PRINT_ANY, NULL, "%s", " "); > close_json_array(PRINT_JSON, NULL); > } >
> Daniel Machon <daniel.machon@microchip.com> writes: > > > -static void dcb_app_print_filtered(const struct dcb_app_table *tab, > > - bool (*filter)(const struct dcb_app *), > > - int (*print_key)(__u16 protocol), > > - const char *json_name, > > - const char *fp_name) > > +void dcb_app_print_filtered(const struct dcb_app_table *tab, > > + bool (*filter)(const struct dcb_app *), > > + int (*print_pid)(__u16 protocol), > > + const char *json_name, const char *fp_name) > > { > > bool first = true; > > size_t i; > > @@ -439,8 +437,14 @@ static void dcb_app_print_filtered(const struct dcb_app_table *tab, > > } > > > > open_json_array(PRINT_JSON, NULL); > > - print_key(app->protocol); > > - print_uint(PRINT_ANY, NULL, "%d ", app->priority); > > + if (tab->attr == DCB_ATTR_IEEE_APP_TABLE) { > > + print_pid(app->protocol); > > + print_uint(PRINT_ANY, NULL, ":%d", app->priority); > > + } else { > > + print_uint(PRINT_ANY, NULL, "%d:", app->priority); > > + print_pid(app->protocol); > > + } > > I really dislike the attribute dispatch. I feels too much like mixing > abstraction layers. I think the callback should take a full struct > dcb_app pointer and format it as appropriate. Then you can model the > rewrite table differently from the app table by providing a callback > that invokes the print_ helpers in the correct order. > > The app->protocol field as such is not really necessary IMHO, because > the function that invokes the helpers understands what kind of table it > is dealing with and could provide it as a parameter. But OK, I guess it > makes sense and probably saves some boilerplate parameterization. Roger. And actually, yeah, the callbacks are used heavily throughout DCB, so that fits better. Will incorporate CB approach in next v. I think this applies more or less to your comments in patch #3, #4 and #5 too :)
<Daniel.Machon@microchip.com> writes: >> Daniel Machon <daniel.machon@microchip.com> writes: >> >> > -static void dcb_app_print_filtered(const struct dcb_app_table *tab, >> > - bool (*filter)(const struct dcb_app *), >> > - int (*print_key)(__u16 protocol), >> > - const char *json_name, >> > - const char *fp_name) >> > +void dcb_app_print_filtered(const struct dcb_app_table *tab, >> > + bool (*filter)(const struct dcb_app *), >> > + int (*print_pid)(__u16 protocol), >> > + const char *json_name, const char *fp_name) >> > { >> > bool first = true; >> > size_t i; >> > @@ -439,8 +437,14 @@ static void dcb_app_print_filtered(const struct dcb_app_table *tab, >> > } >> > >> > open_json_array(PRINT_JSON, NULL); >> > - print_key(app->protocol); >> > - print_uint(PRINT_ANY, NULL, "%d ", app->priority); >> > + if (tab->attr == DCB_ATTR_IEEE_APP_TABLE) { >> > + print_pid(app->protocol); >> > + print_uint(PRINT_ANY, NULL, ":%d", app->priority); >> > + } else { >> > + print_uint(PRINT_ANY, NULL, "%d:", app->priority); >> > + print_pid(app->protocol); >> > + } >> >> I really dislike the attribute dispatch. I feels too much like mixing >> abstraction layers. I think the callback should take a full struct >> dcb_app pointer and format it as appropriate. Then you can model the >> rewrite table differently from the app table by providing a callback >> that invokes the print_ helpers in the correct order. >> >> The app->protocol field as such is not really necessary IMHO, because >> the function that invokes the helpers understands what kind of table it >> is dealing with and could provide it as a parameter. But OK, I guess it >> makes sense and probably saves some boilerplate parameterization. > > Roger. And actually, yeah, the callbacks are used heavily throughout > DCB, so that fits better. Will incorporate CB approach in next v. I > think this applies more or less to your comments in patch #3, #4 and #5 > too :) Yeah, I wasn't sure myself how much of a pain the callback approach brings, so wanted to make sure it's not bending-backwards bad. Hence all that prototype code :)
> On Mon, 22 May 2023 20:41:06 +0200 > Daniel Machon <daniel.machon@microchip.com> wrote: > > > +int dcb_app_print_pid_dec(__u16 protocol) > > { > > - return print_uint(PRINT_ANY, NULL, "%d:", protocol); > > + return print_uint(PRINT_ANY, NULL, "%d", protocol); > > } > > Should be %u for unsigned value. Thanks Stephen, Will make sure to change that in v2. /Daniel
diff --git a/dcb/dcb_app.c b/dcb/dcb_app.c index 1d0da35f987d..9bb64f32e12e 100644 --- a/dcb/dcb_app.c +++ b/dcb/dcb_app.c @@ -389,40 +389,38 @@ static bool dcb_app_is_port(const struct dcb_app *app) return app->selector == IEEE_8021QAZ_APP_SEL_ANY; } -int dcb_app_print_key_dec(__u16 protocol) +int dcb_app_print_pid_dec(__u16 protocol) { - return print_uint(PRINT_ANY, NULL, "%d:", protocol); + return print_uint(PRINT_ANY, NULL, "%d", protocol); } -static int dcb_app_print_key_hex(__u16 protocol) +static int dcb_app_print_pid_hex(__u16 protocol) { - return print_uint(PRINT_ANY, NULL, "%x:", protocol); + return print_uint(PRINT_ANY, NULL, "%x", protocol); } -int dcb_app_print_key_dscp(__u16 protocol) +int dcb_app_print_pid_dscp(__u16 protocol) { const char *name = rtnl_dsfield_get_name(protocol << 2); - if (!is_json_context() && name != NULL) - return print_string(PRINT_FP, NULL, "%s:", name); - return print_uint(PRINT_ANY, NULL, "%d:", protocol); + return print_string(PRINT_FP, NULL, "%s", name); + return print_uint(PRINT_ANY, NULL, "%d", protocol); } -int dcb_app_print_key_pcp(__u16 protocol) +int dcb_app_print_pid_pcp(__u16 protocol) { /* Print in numerical form, if protocol value is out-of-range */ if (protocol > DCB_APP_PCP_MAX) - return print_uint(PRINT_ANY, NULL, "%d:", protocol); + return print_uint(PRINT_ANY, NULL, "%d", protocol); - return print_string(PRINT_ANY, NULL, "%s:", pcp_names[protocol]); + return print_string(PRINT_ANY, NULL, "%s", pcp_names[protocol]); } -static void dcb_app_print_filtered(const struct dcb_app_table *tab, - bool (*filter)(const struct dcb_app *), - int (*print_key)(__u16 protocol), - const char *json_name, - const char *fp_name) +void dcb_app_print_filtered(const struct dcb_app_table *tab, + bool (*filter)(const struct dcb_app *), + int (*print_pid)(__u16 protocol), + const char *json_name, const char *fp_name) { bool first = true; size_t i; @@ -439,8 +437,14 @@ static void dcb_app_print_filtered(const struct dcb_app_table *tab, } open_json_array(PRINT_JSON, NULL); - print_key(app->protocol); - print_uint(PRINT_ANY, NULL, "%d ", app->priority); + if (tab->attr == DCB_ATTR_IEEE_APP_TABLE) { + print_pid(app->protocol); + print_uint(PRINT_ANY, NULL, ":%d", app->priority); + } else { + print_uint(PRINT_ANY, NULL, "%d:", app->priority); + print_pid(app->protocol); + } + print_string(PRINT_ANY, NULL, "%s", " "); close_json_array(PRINT_JSON, NULL); } @@ -452,7 +456,7 @@ static void dcb_app_print_filtered(const struct dcb_app_table *tab, static void dcb_app_print_ethtype_prio(const struct dcb_app_table *tab) { - dcb_app_print_filtered(tab, dcb_app_is_ethtype, dcb_app_print_key_hex, + dcb_app_print_filtered(tab, dcb_app_is_ethtype, dcb_app_print_pid_hex, "ethtype_prio", "ethtype-prio"); } @@ -460,8 +464,8 @@ static void dcb_app_print_pcp_prio(const struct dcb *dcb, const struct dcb_app_table *tab) { dcb_app_print_filtered(tab, dcb_app_is_pcp, - dcb->numeric ? dcb_app_print_key_dec - : dcb_app_print_key_pcp, + dcb->numeric ? dcb_app_print_pid_dec : + dcb_app_print_pid_pcp, "pcp_prio", "pcp-prio"); } @@ -469,26 +473,28 @@ static void dcb_app_print_dscp_prio(const struct dcb *dcb, const struct dcb_app_table *tab) { dcb_app_print_filtered(tab, dcb_app_is_dscp, - dcb->numeric ? dcb_app_print_key_dec - : dcb_app_print_key_dscp, + dcb->numeric ? dcb_app_print_pid_dec : + dcb_app_print_pid_dscp, "dscp_prio", "dscp-prio"); } static void dcb_app_print_stream_port_prio(const struct dcb_app_table *tab) { - dcb_app_print_filtered(tab, dcb_app_is_stream_port, dcb_app_print_key_dec, - "stream_port_prio", "stream-port-prio"); + dcb_app_print_filtered(tab, dcb_app_is_stream_port, + dcb_app_print_pid_dec, "stream_port_prio", + "stream-port-prio"); } static void dcb_app_print_dgram_port_prio(const struct dcb_app_table *tab) { - dcb_app_print_filtered(tab, dcb_app_is_dgram_port, dcb_app_print_key_dec, - "dgram_port_prio", "dgram-port-prio"); + dcb_app_print_filtered(tab, dcb_app_is_dgram_port, + dcb_app_print_pid_dec, "dgram_port_prio", + "dgram-port-prio"); } static void dcb_app_print_port_prio(const struct dcb_app_table *tab) { - dcb_app_print_filtered(tab, dcb_app_is_port, dcb_app_print_key_dec, + dcb_app_print_filtered(tab, dcb_app_is_port, dcb_app_print_pid_dec, "port_prio", "port-prio"); } diff --git a/dcb/dcb_app.h b/dcb/dcb_app.h index 3aea0bfd1786..8f048605c3a8 100644 --- a/dcb/dcb_app.h +++ b/dcb/dcb_app.h @@ -41,9 +41,13 @@ void dcb_app_table_remove_existing(struct dcb_app_table *a, 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_print_pid_dec(__u16 protocol); +int dcb_app_print_pid_dscp(__u16 protocol); +int dcb_app_print_pid_pcp(__u16 protocol); +void dcb_app_print_filtered(const struct dcb_app_table *tab, + bool (*filter)(const struct dcb_app *), + int (*print_pid)(__u16 protocol), + const char *json_name, const char *fp_name); int dcb_app_parse_pcp(__u32 *key, const char *arg); int dcb_app_parse_dscp(__u32 *key, const char *arg);
Whereas dcb-app requires protocol to be the printed key, dcb-rewr requires it to be the priority. Existing dcb-app print functions can easily be adapted to support this, by using the newly introduced dcbnl attribute in the dcb_app_table struct. - dcb_app_print_key_*() functions have been renamed to dcb_app_print_pid_*() to align with new situation. Also, none of them will print the colon anymore. - dcb_app_print_filtered() will now print either priority or protocol first, depending on the dcbnl set/get attribute. Signed-off-by: Daniel Machon <daniel.machon@microchip.com> --- dcb/dcb_app.c | 62 ++++++++++++++++++++++++++++++++--------------------------- dcb/dcb_app.h | 10 +++++++--- 2 files changed, 41 insertions(+), 31 deletions(-)