| Message ID | 20230510-dcb-rewr-v2-2-9f38e688117e@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 |
Daniel Machon <daniel.machon@microchip.com> writes: > Where dcb-app requires protocol to be the printed key, dcb-rewr requires > it to be the priority. Adapt existing dcb-app print functions for this. > > dcb_app_print_filtered() has been modified, to take two callbacks; one > for printing the entire string (pid and prio), and one for the pid type > (dec, hex, dscp, pcp). This saves us for making one dedicated function > for each pid type for both app and rewr. > > 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. > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> There are about four patches included in this one patch: the %d->%u change, the colon shenanigans, the renaming, and prototype change of dcb_app_print_filtered(). I think the code is OK, but I would appreciate splitting into a patch per feature.
> > Where dcb-app requires protocol to be the printed key, dcb-rewr requires > > it to be the priority. Adapt existing dcb-app print functions for this. > > > > dcb_app_print_filtered() has been modified, to take two callbacks; one > > for printing the entire string (pid and prio), and one for the pid type > > (dec, hex, dscp, pcp). This saves us for making one dedicated function > > for each pid type for both app and rewr. > > > > 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. > > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > There are about four patches included in this one patch: the %d->%u > change, the colon shenanigans, the renaming, and prototype change of > dcb_app_print_filtered(). > > I think the code is OK, but I would appreciate splitting into a patch > per feature. Sure I can split those changes up.
> Daniel Machon <daniel.machon@microchip.com> writes: > > > Where dcb-app requires protocol to be the printed key, dcb-rewr requires > > it to be the priority. Adapt existing dcb-app print functions for this. > > > > dcb_app_print_filtered() has been modified, to take two callbacks; one > > for printing the entire string (pid and prio), and one for the pid type > > (dec, hex, dscp, pcp). This saves us for making one dedicated function > > for each pid type for both app and rewr. > > > > 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. > > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > There are about four patches included in this one patch: the %d->%u > change, the colon shenanigans, the renaming, and prototype change of > dcb_app_print_filtered(). > > I think the code is OK, but I would appreciate splitting into a patch > per feature. Actually, IMHO, the 'colon shenanigans' does not make much sense in a patch of its own, since its not really a standalone feature, but rather some change that is required for the prototype change of dcb_app_print_filtered?
Daniel Machon <daniel.machon@microchip.com> writes: >> Daniel Machon <daniel.machon@microchip.com> writes: >> >> > Where dcb-app requires protocol to be the printed key, dcb-rewr requires >> > it to be the priority. Adapt existing dcb-app print functions for this. >> > >> > dcb_app_print_filtered() has been modified, to take two callbacks; one >> > for printing the entire string (pid and prio), and one for the pid type >> > (dec, hex, dscp, pcp). This saves us for making one dedicated function >> > for each pid type for both app and rewr. >> > >> > 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. >> > >> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> >> >> There are about four patches included in this one patch: the %d->%u >> change, the colon shenanigans, the renaming, and prototype change of >> dcb_app_print_filtered(). >> >> I think the code is OK, but I would appreciate splitting into a patch >> per feature. > > Actually, IMHO, the 'colon shenanigans' does not make much sense in a > patch of its own, since its not really a standalone feature, but rather > some change that is required for the prototype change of > dcb_app_print_filtered? It doesn't make sense on its own, but it also does not get sent on its own, but together with the following patches. Its role is to set stage. dcb_app_print_filtered() should first stop assuming the callback prints a colon. So you have one patch where the colon moves from the callback to _print_filtered(). That's a clean, behavior-neutral change that should be trivial to review. Then next patch introduces the callback. Which at that point should likewise be tidy, focused and easy to review, because it will only deal with the new callback.
On 5/31/23 5:26 AM, Petr Machata wrote: > It doesn't make sense on its own, but it also does not get sent on its > own, but together with the following patches. Its role is to set stage. > > dcb_app_print_filtered() should first stop assuming the callback prints > a colon. So you have one patch where the colon moves from the callback > to _print_filtered(). That's a clean, behavior-neutral change that > should be trivial to review. Then next patch introduces the callback. > Which at that point should likewise be tidy, focused and easy to review, > because it will only deal with the new callback. agreed. code should evolve in a way that makes it easy to review.
diff --git a/dcb/dcb_app.c b/dcb/dcb_app.c index 8073415ad084..1e36541bec61 100644 --- a/dcb/dcb_app.c +++ b/dcb/dcb_app.c @@ -403,38 +403,39 @@ 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) +static int dcb_app_print_pid_dec(__u16 protocol) { - return print_uint(PRINT_ANY, NULL, "%d:", protocol); + return print_uint(PRINT_ANY, NULL, "%u", 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); } -static int dcb_app_print_key_dscp(__u16 protocol) +static 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, "%u", protocol); } -static int dcb_app_print_key_pcp(__u16 protocol) +static 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, "%u", 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), + void (*print_pid_prio)(int (*print_pid)(__u16), + const struct dcb_app *), + int (*print_pid)(__u16 protocol), const char *json_name, const char *fp_name) { @@ -453,8 +454,8 @@ 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); + print_pid_prio(print_pid, app); + print_string(PRINT_ANY, NULL, "%s", " "); close_json_array(PRINT_JSON, NULL); } @@ -464,9 +465,17 @@ static void dcb_app_print_filtered(const struct dcb_app_table *tab, } } +static void dcb_app_print_pid_prio(int (*print_pid)(__u16 protocol), + const struct dcb_app *app) +{ + print_pid(app->protocol); + print_uint(PRINT_ANY, NULL, ":%u", app->priority); +} + 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_prio, dcb_app_print_pid_hex, "ethtype_prio", "ethtype-prio"); } @@ -474,8 +483,9 @@ 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_app_print_pid_prio, + dcb->numeric ? dcb_app_print_pid_dec : + dcb_app_print_pid_pcp, "pcp_prio", "pcp-prio"); } @@ -483,26 +493,30 @@ 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_app_print_pid_prio, + 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, + dcb_app_print_filtered(tab, dcb_app_is_stream_port, + dcb_app_print_pid_prio, 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, + dcb_app_print_filtered(tab, dcb_app_is_dgram_port, + dcb_app_print_pid_prio, 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_prio, dcb_app_print_pid_dec, "port_prio", "port-prio"); }
Where dcb-app requires protocol to be the printed key, dcb-rewr requires it to be the priority. Adapt existing dcb-app print functions for this. dcb_app_print_filtered() has been modified, to take two callbacks; one for printing the entire string (pid and prio), and one for the pid type (dec, hex, dscp, pcp). This saves us for making one dedicated function for each pid type for both app and rewr. 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. Signed-off-by: Daniel Machon <daniel.machon@microchip.com> --- dcb/dcb_app.c | 58 ++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 22 deletions(-)