On 3/26/25 13:58, Eelco Chaudron wrote:
>
>
> 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 <ovs-dev@openvswitch.org> writes:
>>>>
>>>>> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>>>>> ---
>>>>
>>>> LGTM.
>>>>
>>>> Acked-by: Aaron Conole <acon...@redhat.com>
>>>>
>>>> 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");
>
Maybe something like this:
ds_put_cstr(ds,
"Can not enable features not supported by the datapth");
?
Not ideal, but seems better than other options.
>>>>> }
>>>>> } else if (!strcasecmp(value, "false")) {
>>>>> *(bool *)field->rt_ptr = false;
>>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
