On 04/11/2022 0:48, Justin Pettit wrote:
>
>> On Nov 3, 2022, at 3:38 PM, Ilya Maximets <i.maxim...@ovn.org> wrote:
>>
>> On 11/3/22 09:47, Roi Dayan wrote:
>>>
>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>>> index a620a6ec52dd..2bdd2137af36 100644
>>> --- a/lib/dpif-netlink.c
>>> +++ b/lib/dpif-netlink.c
>>> @@ -4105,7 +4105,7 @@ dpif_netlink_meter_get_features(const struct dpif
>>> *dpif_,
>>> struct ofputil_meter_features *features)
>>> {
>>> if (probe_broken_meters(CONST_CAST(struct dpif *, dpif_))) {
>>> - features = NULL;
>>> + memset(features, 0, sizeof *features);> return;
>>> }
>>>
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index 40f5fe44606e..f00aeea37428 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -1935,7 +1935,6 @@ void
>>> dpif_meter_get_features(const struct dpif *dpif,
>>> struct ofputil_meter_features *features)
>>> {
>>> - memset(features, 0, sizeof *features);
>>
>> I'm not sure this is correct, because we should still clear them
>> even if the dpif_class doesn't have the callback.
>>
>> All providers seem to implement this callback, but not clearing
>> features doesn't seem correct from the API clarity point of view.
>>
>> Just returning without clearing the structure in
>> dpif_netlink_meter_get_features() might be a better option.
>> Clearing in both places is also fine as it's not a performance
>> critical code path.
>
> Agreed. This is the point I was trying to make in my original feedback: I
> think the patch can just remove the "features = NULL;" line in
> dpif_netlink_meter_get_features() and leave everything else as-is. So the
> patch essentially becomes, since dpif_meter_get_features() already cleared
> out 'features':
>
> -=-=-=-=-=-=-=-
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -4105,7 +4105,6 @@ dpif_netlink_meter_get_features(const struct dpif
> *dpif_,
> struct ofputil_meter_features *features)
> {
> if (probe_broken_meters(CONST_CAST(struct dpif *, dpif_))) {
> - features = NULL;
> return;
> }
> -=-=-=-=-=-=-=-
>
> --Justin
>
>
ok i see. in ct get features it's different as the function is not
void and returns not supported if callback doesn't exists.
i'll fix the commit as suggested. thanks.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
