On Tue, Apr 16, 2024 at 04:21:48PM +0300, Roi Dayan via dev wrote: > VLOG_WARN_BUF() is allocating memory for the error string and should > e used if the configuration cannot continue and error is being returned > so the caller has indication of releasing the pointer. > Change to VLOG_WARN() to keep the logic that error is not being > returned. > > Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address.") > Signed-off-by: Roi Dayan <r...@nvidia.com> > Acked-by: Gaetan Rivet <gaet...@nvidia.com> > Acked-by: Eli Britstein <el...@nvidia.com> > --- > lib/netdev-dpdk.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 2111f776810b..9249b9e9c646 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2379,17 +2379,16 @@ netdev_dpdk_set_config(struct netdev *netdev, const > struct smap *args, > struct eth_addr mac; > > if (!dpdk_port_is_representor(dev)) { > - VLOG_WARN_BUF(errp, "'%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); > + 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_BUF(errp, "interface '%s': cannot parse VF MAC '%s'.", > - netdev_get_name(netdev), vf_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_BUF(errp, > - "interface '%s': cannot set VF MAC to multicast " > - "address '%s'.", netdev_get_name(netdev), vf_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);
Thanks Roi, I agree that this change makes sense as the allocated value will typically be discarded. And I think if VLOG_WARN_BUF() is called again in the same invocation of netdev_dpdk_set_config() a memory leak occurs. Acked-by: Simon Horman <ho...@ovn.org> After reviewing your patch I am now wondering if, conversely, the following change _also_ makes sense: diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 2111f776810b..32d4193d24af 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2426,8 +2426,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, } err = 0; /* Not fatal. */ } else { - VLOG_WARN("%s: Cannot get flow control parameters: %s", - netdev_get_name(netdev), rte_strerror(err)); + VLOG_WARN_BUF(errp, "%s: Cannot get flow control parameters: %s", + netdev_get_name(netdev), rte_strerror(err)); } goto out; } _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev