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?
>
>>> 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.
>>> 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.
>>> }
>>> } else if (!strcasecmp(value, "false")) {
>>> *(bool *)field->rt_ptr = false;
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
