On Mon, Jun 17, 2024 at 10:11 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> 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.
>
Talk about obvious...
I'll fix my stuff and leave the rest untouched.
--
David Marchand
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
