On 26 Mar 2025, at 13:20, Ilya Maximets wrote:
> On 3/25/25 16:39, Eelco Chaudron wrote: >> >> >> On 25 Mar 2025, at 16:29, Aaron Conole wrote: >> >>> Eelco Chaudron via dev <[email protected]> writes: >>> >>>> Signed-off-by: Eelco Chaudron <[email protected]> >>>> --- >>> >>> LGTM. >>> >>> Acked-by: Aaron Conole <[email protected]> >>> >>> BTW, do you think we should turn on the spell checker in the robot? >>> Currently it is off because there are some false positives that still >>> get flagged. >> >> Thanks for the review! It might be a good trigger to update our word list, >> and if we do get too many reports, we can always turn it off. > > I'm not sure about this. We hit unknown to the word list terms > regularly still. There are lots of them. We need some better > heuristics for detecting words that should not be checked, like > function and variable names and module names in patch subjects. > > For the patch itself, could you, please, come up with a little > more descriptive subject for it? It's not good to have commits > named the same way in the history and there is a high chance > will may have another one like this in the future. Maybe something > like: > > ofproto-dpif: Fix spelling in comments and the support field macro. > > WDYT? Sounds good, I’ll send a v3. >>>> ofproto/ofproto-dpif.c | 45 +++++++++++++++++++++--------------------- >>>> 1 file changed, 23 insertions(+), 22 deletions(-) >>>> >>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>>> index 25b1d9322..06f6d7be3 100644 >>>> --- a/ofproto/ofproto-dpif.c >>>> +++ b/ofproto/ofproto-dpif.c >>>> @@ -102,7 +102,7 @@ struct ofbundle { >>>> * NULL if all VLANs are trunked. */ >>>> unsigned long *cvlans; >>>> struct lacp *lacp; /* LACP if LACP is enabled, otherwise >>>> NULL. */ >>>> - struct bond *bond; /* Nonnull iff more than one port. */ >>>> + struct bond *bond; /* Nonnull if more than one port. */ >>>> enum port_priority_tags_mode use_priority_tags; >>>> /* Use 802.1p tag for frames in VLAN 0? */ >>>> >>>> @@ -1508,7 +1508,7 @@ check_max_dp_hash_alg(struct dpif_backer *backer) >>>> ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); >>>> odp_flow_key_from_flow(&odp_parms, &key); >>>> >>>> - /* All datapaths support algortithm 0 (OVS_HASH_ALG_L4). */ >>>> + /* All datapaths support algorithm 0 (OVS_HASH_ALG_L4). */ >>>> for (int alg = 1; alg < __OVS_HASH_MAX; alg++) { >>>> struct ofpbuf actions; >>>> bool ok; >>>> @@ -3642,7 +3642,7 @@ bundle_set(struct ofproto *ofproto_, void *aux, >>>> bundle->bond = NULL; >>>> } >>>> >>>> - /* Set proteced port mode */ >>>> + /* Set protected port mode */ > > While we're here, may as well add a period to the end. +1 > >>>> if (s->protected != bundle->protected) { >>>> bundle->protected = s->protected; >>>> need_flush = true; >>>> @@ -4609,7 +4609,7 @@ ofproto_dpif_credit_table_stats(struct ofproto_dpif >>>> *ofproto, uint8_t table_id, >>>> >>>> /* Look up 'flow' in 'ofproto''s classifier version 'version', starting >>>> from >>>> * table '*table_id'. Returns the rule that was found, which may be one >>>> of the >>>> - * special rules according to packet miss hadling. If 'may_packet_in' is >>>> + * special rules according to packet miss handling. If 'may_packet_in' is >>>> * false, returning of the miss_rule (which issues packet ins for the >>>> * controller) is avoided. Updates 'wc', if nonnull, to reflect the >>>> fields >>>> * that were used during the lookup. >>>> @@ -6535,7 +6535,7 @@ struct dpif_support_field { >>>> enum dpif_support_field_type type; >>>> }; >>>> >>>> -#define DPIF_SUPPORT_FIELD_INTIALIZER(RT_PTR, BT_PTR, TITLE, TYPE) \ >>>> +#define DPIF_SUPPORT_FIELD_INITIALIZER(RT_PTR, BT_PTR, TITLE, TYPE) \ >>>> (struct dpif_support_field) {RT_PTR, BT_PTR, TITLE, TYPE} >>>> >>>> static void >>>> @@ -6601,26 +6601,26 @@ dpif_set_support(struct dpif_backer_support >>>> *rt_support, >>>> struct shash_node *node; >>>> bool changed = false; >>>> >>>> -#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ >>>> - {\ >>>> - struct dpif_support_field *f = xmalloc(sizeof *f); \ >>>> - *f = DPIF_SUPPORT_FIELD_INTIALIZER(&rt_support->NAME, \ >>>> - &bt_support->NAME, \ >>>> - TITLE, \ >>>> - DPIF_SUPPORT_FIELD_##TYPE);\ >>>> - shash_add_once(&all_fields, #NAME, f); \ >>>> +#define DPIF_SUPPORT_FIELD(TYPE, NAME, TITLE) \ >>>> + { \ >>>> + struct dpif_support_field *f = xmalloc(sizeof *f); \ >>>> + *f = DPIF_SUPPORT_FIELD_INITIALIZER(&rt_support->NAME, \ >>>> + &bt_support->NAME, \ >>>> + TITLE, \ >>>> + DPIF_SUPPORT_FIELD_##TYPE); \ >>>> + shash_add_once(&all_fields, #NAME, f); \ >>>> } >>>> DPIF_SUPPORT_FIELDS; >>>> #undef DPIF_SUPPORT_FIELD >>>> >>>> -#define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \ >>>> - {\ >>>> - struct dpif_support_field *f = xmalloc(sizeof *f); \ >>>> - *f = DPIF_SUPPORT_FIELD_INTIALIZER(&rt_support->odp.NAME, \ >>>> - &bt_support->odp.NAME, \ >>>> - TITLE, \ >>>> - DPIF_SUPPORT_FIELD_##TYPE);\ >>>> - shash_add_once(&all_fields, #NAME, f); \ >>>> +#define ODP_SUPPORT_FIELD(TYPE, NAME, TITLE) \ >>>> + { \ >>>> + struct dpif_support_field *f = xmalloc(sizeof *f); \ >>>> + *f = DPIF_SUPPORT_FIELD_INITIALIZER(&rt_support->odp.NAME, \ >>>> + &bt_support->odp.NAME, \ >>>> + TITLE, \ >>>> + DPIF_SUPPORT_FIELD_##TYPE); \ >>>> + shash_add_once(&all_fields, #NAME, f); \ >>>> } >>>> ODP_SUPPORT_FIELDS; >>>> #undef ODP_SUPPORT_FIELD >>>> @@ -6655,7 +6655,8 @@ dpif_set_support(struct dpif_backer_support >>>> *rt_support, >>>> *(bool *) field->rt_ptr = true; >>>> changed = true; >>>> } else { >>>> - ds_put_cstr(ds, "Can not enable features not supported by >>>> the datapth"); >>>> + ds_put_cstr(ds, "Can not enable features not supported by >>>> " >>>> + "the datapath"); > > I'd suggest to move the whole thing to the new line instead of breaking it. > Seems inconsistent with the rest of the function. But that it would become (to still meet the line lenght): ds_put_cstr(ds, "Can not enable features not supported by the " "datapath"); >>>> } >>>> } else if (!strcasecmp(value, "false")) { >>>> *(bool *)field->rt_ptr = false; >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
