| Message ID | 20230510-dcb-rewr-v2-4-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: > When parsing APP table entries, priority and protocol is assigned from > value and key, respectively. Rewrite requires it opposite. > > Adapt the existing dcb_app_parse_mapping_cb for this, by using callbacks > for pushing app or rewr entries to the table. > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > --- > dcb/dcb.h | 12 ++++++++++++ > dcb/dcb_app.c | 23 ++++++++++++----------- > 2 files changed, 24 insertions(+), 11 deletions(-) > > diff --git a/dcb/dcb.h b/dcb/dcb.h > index 84ce95d5c1b2..b3bc30cd02c5 100644 > --- a/dcb/dcb.h > +++ b/dcb/dcb.h > @@ -62,7 +62,16 @@ struct dcb_app_table { > int attr; > }; > > +struct dcb_app_parse_mapping { > + __u8 selector; > + struct dcb_app_table *tab; > + int (*push)(struct dcb_app_table *tab, > + __u8 selector, __u32 key, __u64 value); > + int err; > +}; > + > 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); > @@ -70,11 +79,14 @@ bool dcb_app_selector_validate(enum ieee_attrs_app type, __u8 selector); > bool dcb_app_pid_eq(const struct dcb_app *aa, const struct dcb_app *ab); > bool dcb_app_prio_eq(const struct dcb_app *aa, const struct dcb_app *ab); > > +int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app); > void dcb_app_table_remove_replaced(struct dcb_app_table *a, > const struct dcb_app_table *b, > bool (*key_eq)(const struct dcb_app *aa, > const struct dcb_app *ab)); > > +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data); > + > /* 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 4cd175a0623b..97cba658aa6b 100644 > --- a/dcb/dcb_app.c > +++ b/dcb/dcb_app.c > @@ -105,7 +105,7 @@ static void dcb_app_table_fini(struct dcb_app_table *tab) > free(tab->apps); > } > > -static int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app) > +int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app) > { > struct dcb_app *apps = realloc(tab->apps, (tab->n_apps + 1) * sizeof(*tab->apps)); > > @@ -231,25 +231,25 @@ static 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) > +static int dcb_app_push(struct dcb_app_table *tab, > + __u8 selector, __u32 key, __u64 value) > { > - struct dcb_app_parse_mapping *pm = data; > struct dcb_app app = { > - .selector = pm->selector, > + .selector = selector, > .priority = value, > .protocol = key, > }; > + return dcb_app_table_push(tab, &app); > +} > + > +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data) > +{ > + struct dcb_app_parse_mapping *pm = data; > > if (pm->err) > return; > > - pm->err = dcb_app_table_push(pm->tab, &app); > + pm->err = pm->push(pm->tab, pm->selector, key, value); > } > > static int dcb_app_parse_mapping_ethtype_prio(__u32 key, char *value, void *data) > @@ -663,6 +663,7 @@ static int dcb_cmd_app_parse_add_del(struct dcb *dcb, const char *dev, > { > struct dcb_app_parse_mapping pm = { > .tab = tab, > + .push = dcb_app_push, > }; > int ret; I think I misunderstood your code. Since you are adding new functions for parsing the PRIO-DSCP and PRIO-PCP mappings, which have their own dcb_parse_mapping() invocations, couldn't you just copy over the dcb_app_parse_mapping_cb() from APP and adapt it to do the right thing for REWR? Then the push callback is not even necessary dcb_app_parse_mapping_cb() does not need to be public.
> Daniel Machon <daniel.machon@microchip.com> writes: > > > When parsing APP table entries, priority and protocol is assigned from > > value and key, respectively. Rewrite requires it opposite. > > > > Adapt the existing dcb_app_parse_mapping_cb for this, by using callbacks > > for pushing app or rewr entries to the table. > > > > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> > > --- > > dcb/dcb.h | 12 ++++++++++++ > > dcb/dcb_app.c | 23 ++++++++++++----------- > > 2 files changed, 24 insertions(+), 11 deletions(-) > > > > diff --git a/dcb/dcb.h b/dcb/dcb.h > > index 84ce95d5c1b2..b3bc30cd02c5 100644 > > --- a/dcb/dcb.h > > +++ b/dcb/dcb.h > > @@ -62,7 +62,16 @@ struct dcb_app_table { > > int attr; > > }; > > > > +struct dcb_app_parse_mapping { > > + __u8 selector; > > + struct dcb_app_table *tab; > > + int (*push)(struct dcb_app_table *tab, > > + __u8 selector, __u32 key, __u64 value); > > + int err; > > +}; > > + > > 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); > > @@ -70,11 +79,14 @@ bool dcb_app_selector_validate(enum ieee_attrs_app type, __u8 selector); > > bool dcb_app_pid_eq(const struct dcb_app *aa, const struct dcb_app *ab); > > bool dcb_app_prio_eq(const struct dcb_app *aa, const struct dcb_app *ab); > > > > +int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app); > > void dcb_app_table_remove_replaced(struct dcb_app_table *a, > > const struct dcb_app_table *b, > > bool (*key_eq)(const struct dcb_app *aa, > > const struct dcb_app *ab)); > > > > +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data); > > + > > /* 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 4cd175a0623b..97cba658aa6b 100644 > > --- a/dcb/dcb_app.c > > +++ b/dcb/dcb_app.c > > @@ -105,7 +105,7 @@ static void dcb_app_table_fini(struct dcb_app_table *tab) > > free(tab->apps); > > } > > > > -static int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app) > > +int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app) > > { > > struct dcb_app *apps = realloc(tab->apps, (tab->n_apps + 1) * sizeof(*tab->apps)); > > > > @@ -231,25 +231,25 @@ static 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) > > +static int dcb_app_push(struct dcb_app_table *tab, > > + __u8 selector, __u32 key, __u64 value) > > { > > - struct dcb_app_parse_mapping *pm = data; > > struct dcb_app app = { > > - .selector = pm->selector, > > + .selector = selector, > > .priority = value, > > .protocol = key, > > }; > > + return dcb_app_table_push(tab, &app); > > +} > > + > > +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data) > > +{ > > + struct dcb_app_parse_mapping *pm = data; > > > > if (pm->err) > > return; > > > > - pm->err = dcb_app_table_push(pm->tab, &app); > > + pm->err = pm->push(pm->tab, pm->selector, key, value); > > } > > > > static int dcb_app_parse_mapping_ethtype_prio(__u32 key, char *value, void *data) > > @@ -663,6 +663,7 @@ static int dcb_cmd_app_parse_add_del(struct dcb *dcb, const char *dev, > > { > > struct dcb_app_parse_mapping pm = { > > .tab = tab, > > + .push = dcb_app_push, > > }; > > int ret; > > I think I misunderstood your code. Since you are adding new functions > for parsing the PRIO-DSCP and PRIO-PCP mappings, which have their own > dcb_parse_mapping() invocations, couldn't you just copy over the > dcb_app_parse_mapping_cb() from APP and adapt it to do the right thing > for REWR? Then the push callback is not even necessary > dcb_app_parse_mapping_cb() does not need to be public. It is always a balance of when to do what. So far, patches #2, #3 and #4 tries to modify the existing dcb-app functions for dcb-rewr reuse. They all deal with the prio:pid, pid:prio problem (printing, pushing and replacing entries). What you suggest now is to copy dcb_app_parse_mapping_cb() entirely, just for changing that order. It can be done, but then it could also be done for #2 and #3, which would then result in more boilerplate code. Whatever we choose, I think we should stay consistent?
Daniel Machon <daniel.machon@microchip.com> writes: >> Daniel Machon <daniel.machon@microchip.com> writes: >> >> > When parsing APP table entries, priority and protocol is assigned from >> > value and key, respectively. Rewrite requires it opposite. >> > >> > Adapt the existing dcb_app_parse_mapping_cb for this, by using callbacks >> > for pushing app or rewr entries to the table. >> > >> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com> >> > --- >> > dcb/dcb.h | 12 ++++++++++++ >> > dcb/dcb_app.c | 23 ++++++++++++----------- >> > 2 files changed, 24 insertions(+), 11 deletions(-) >> > >> > diff --git a/dcb/dcb.h b/dcb/dcb.h >> > index 84ce95d5c1b2..b3bc30cd02c5 100644 >> > --- a/dcb/dcb.h >> > +++ b/dcb/dcb.h >> > @@ -62,7 +62,16 @@ struct dcb_app_table { >> > int attr; >> > }; >> > >> > +struct dcb_app_parse_mapping { >> > + __u8 selector; >> > + struct dcb_app_table *tab; >> > + int (*push)(struct dcb_app_table *tab, >> > + __u8 selector, __u32 key, __u64 value); >> > + int err; >> > +}; >> > + >> > 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); >> > @@ -70,11 +79,14 @@ bool dcb_app_selector_validate(enum ieee_attrs_app type, __u8 selector); >> > bool dcb_app_pid_eq(const struct dcb_app *aa, const struct dcb_app *ab); >> > bool dcb_app_prio_eq(const struct dcb_app *aa, const struct dcb_app *ab); >> > >> > +int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app); >> > void dcb_app_table_remove_replaced(struct dcb_app_table *a, >> > const struct dcb_app_table *b, >> > bool (*key_eq)(const struct dcb_app *aa, >> > const struct dcb_app *ab)); >> > >> > +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data); >> > + >> > /* 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 4cd175a0623b..97cba658aa6b 100644 >> > --- a/dcb/dcb_app.c >> > +++ b/dcb/dcb_app.c >> > @@ -105,7 +105,7 @@ static void dcb_app_table_fini(struct dcb_app_table *tab) >> > free(tab->apps); >> > } >> > >> > -static int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app) >> > +int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app) >> > { >> > struct dcb_app *apps = realloc(tab->apps, (tab->n_apps + 1) * sizeof(*tab->apps)); >> > >> > @@ -231,25 +231,25 @@ static 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) >> > +static int dcb_app_push(struct dcb_app_table *tab, >> > + __u8 selector, __u32 key, __u64 value) >> > { >> > - struct dcb_app_parse_mapping *pm = data; >> > struct dcb_app app = { >> > - .selector = pm->selector, >> > + .selector = selector, >> > .priority = value, >> > .protocol = key, >> > }; >> > + return dcb_app_table_push(tab, &app); >> > +} >> > + >> > +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data) >> > +{ >> > + struct dcb_app_parse_mapping *pm = data; >> > >> > if (pm->err) >> > return; >> > >> > - pm->err = dcb_app_table_push(pm->tab, &app); >> > + pm->err = pm->push(pm->tab, pm->selector, key, value); >> > } >> > >> > static int dcb_app_parse_mapping_ethtype_prio(__u32 key, char *value, void *data) >> > @@ -663,6 +663,7 @@ static int dcb_cmd_app_parse_add_del(struct dcb *dcb, const char *dev, >> > { >> > struct dcb_app_parse_mapping pm = { >> > .tab = tab, >> > + .push = dcb_app_push, >> > }; >> > int ret; >> >> I think I misunderstood your code. Since you are adding new functions >> for parsing the PRIO-DSCP and PRIO-PCP mappings, which have their own >> dcb_parse_mapping() invocations, couldn't you just copy over the >> dcb_app_parse_mapping_cb() from APP and adapt it to do the right thing >> for REWR? Then the push callback is not even necessary >> dcb_app_parse_mapping_cb() does not need to be public. > > It is always a balance of when to do what. So far, patches #2, #3 and #4 > tries to modify the existing dcb-app functions for dcb-rewr reuse. They > all deal with the prio:pid, pid:prio problem (printing, pushing and > replacing entries). What you suggest now is to copy > dcb_app_parse_mapping_cb() entirely, just for changing that order. It > can be done, but then it could also be done for #2 and #3, which would > then result in more boilerplate code. > > Whatever we choose, I think we should stay consistent? I mean, where do you put the threshold? Because what currently gets reused is this: void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data) { struct dcb_app_parse_mapping *pm = data; if (pm->err) return; pm->err = pm->push(pm->tab, pm->selector, key, value); } (OK, that, and the helper data structure) IMHO the ceremony around the declaration, it not being near where it's used, the extra callback to make it generic, etc., is more expensive than what the reuse saves us. Like, similarly we could talk about reusing dcb_cmd_app() for dcb_rewr_app() or whatever. But we shamelessly duplicate, because making it reusable is more expensive than what it brings. If anything, I would reuse the data structure, and copy the callback.
diff --git a/dcb/dcb.h b/dcb/dcb.h index 84ce95d5c1b2..b3bc30cd02c5 100644 --- a/dcb/dcb.h +++ b/dcb/dcb.h @@ -62,7 +62,16 @@ struct dcb_app_table { int attr; }; +struct dcb_app_parse_mapping { + __u8 selector; + struct dcb_app_table *tab; + int (*push)(struct dcb_app_table *tab, + __u8 selector, __u32 key, __u64 value); + int err; +}; + 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); @@ -70,11 +79,14 @@ bool dcb_app_selector_validate(enum ieee_attrs_app type, __u8 selector); bool dcb_app_pid_eq(const struct dcb_app *aa, const struct dcb_app *ab); bool dcb_app_prio_eq(const struct dcb_app *aa, const struct dcb_app *ab); +int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app); void dcb_app_table_remove_replaced(struct dcb_app_table *a, const struct dcb_app_table *b, bool (*key_eq)(const struct dcb_app *aa, const struct dcb_app *ab)); +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data); + /* 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 4cd175a0623b..97cba658aa6b 100644 --- a/dcb/dcb_app.c +++ b/dcb/dcb_app.c @@ -105,7 +105,7 @@ static void dcb_app_table_fini(struct dcb_app_table *tab) free(tab->apps); } -static int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app) +int dcb_app_table_push(struct dcb_app_table *tab, struct dcb_app *app) { struct dcb_app *apps = realloc(tab->apps, (tab->n_apps + 1) * sizeof(*tab->apps)); @@ -231,25 +231,25 @@ static 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) +static int dcb_app_push(struct dcb_app_table *tab, + __u8 selector, __u32 key, __u64 value) { - struct dcb_app_parse_mapping *pm = data; struct dcb_app app = { - .selector = pm->selector, + .selector = selector, .priority = value, .protocol = key, }; + return dcb_app_table_push(tab, &app); +} + +void dcb_app_parse_mapping_cb(__u32 key, __u64 value, void *data) +{ + struct dcb_app_parse_mapping *pm = data; if (pm->err) return; - pm->err = dcb_app_table_push(pm->tab, &app); + pm->err = pm->push(pm->tab, pm->selector, key, value); } static int dcb_app_parse_mapping_ethtype_prio(__u32 key, char *value, void *data) @@ -663,6 +663,7 @@ static int dcb_cmd_app_parse_add_del(struct dcb *dcb, const char *dev, { struct dcb_app_parse_mapping pm = { .tab = tab, + .push = dcb_app_push, }; int ret;
When parsing APP table entries, priority and protocol is assigned from value and key, respectively. Rewrite requires it opposite. Adapt the existing dcb_app_parse_mapping_cb for this, by using callbacks for pushing app or rewr entries to the table. Signed-off-by: Daniel Machon <daniel.machon@microchip.com> --- dcb/dcb.h | 12 ++++++++++++ dcb/dcb_app.c | 23 ++++++++++++----------- 2 files changed, 24 insertions(+), 11 deletions(-)