On 6/17/24 09:46, David Marchand wrote:
> On Fri, Jun 14, 2024 at 6:22 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index 0fa37d5145..a260bc8485 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -2397,7 +2397,18 @@ netdev_dpdk_set_config(struct netdev *netdev, const
>>> struct smap *args,
>>> }
>>> }
>>>
>>> - lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", false);
>>> + lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", true);
>>> + if (lsc_interrupt_mode && !(*info.dev_flags & RTE_ETH_DEV_INTR_LSC)) {
>>> + if (smap_get(args, "dpdk-lsc-interrupt")) {
>>> + VLOG_ERR("interface '%s': link status interrupt is not
>>> supported.",
>>> + netdev_get_name(netdev));
>>
>> Since we're exiting with an error set, the message should be buffered
>> into errp instead, so it can be visible in the database record and
>> returned as a result of the ovs-vsctl.
>>
>> Also, we're using WARN level for all other configuration issues, so we
>> should do that here as well. ERR is usually some sort of internal error.
>> And we're usually just using "%s: ..." and not "interface '%s': ...".
>
> Ok for ERR vs WARN.
>
> For the rest, well, I copied the logs right before.
>
> vf_mac = smap_get(args, "dpdk-vf-mac");
> if (vf_mac) {
> struct eth_addr mac;
>
> if (!dpdk_port_is_representor(dev)) {
> VLOG_WARN("'%s' is trying to set the VF MAC '%s' "
> "but 'options:dpdk-vf-mac' is only supported for "
> "VF representors.",
> netdev_get_name(netdev), vf_mac);
> } else if (!eth_addr_from_string(vf_mac, &mac)) {
> VLOG_WARN("interface '%s': cannot parse VF MAC '%s'.",
> netdev_get_name(netdev), vf_mac);
> } else if (eth_addr_is_multicast(mac)) {
> VLOG_WARN("interface '%s': cannot set VF MAC to multicast "
> "address '%s'.", netdev_get_name(netdev), vf_mac);
> } else if (!eth_addr_equals(dev->requested_hwaddr, mac)) {
> dev->requested_hwaddr = mac;
> netdev_request_reconfigure(netdev);
> }
> }
>
> lsc_interrupt_mode = smap_get_bool(args, "dpdk-lsc-interrupt", true);
>
>
> So I'll fix the dpdk-vf-mac stuff (and double check the rest of this
> function), then go with your suggestion for this added log of mine.
>
>
We must not initialize errp if we do not fail with error, otherwise we leak
the memory. VF mac code does not fail the configuration, so we only log the
warning. All the paths that fail should set errp instead.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
