On Wed, Apr 17, 2024 at 10:43:11AM +0300, Roi Dayan wrote:
>
>
> On 16/04/2024 18:48, Simon Horman wrote:
> > 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 <[email protected]>
> >> Acked-by: Gaetan Rivet <[email protected]>
> >> Acked-by: Eli Britstein <[email protected]>
> >> ---
> >> 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 <[email protected]>
> >
> > 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;
> > }
>
> Hi,
>
> thanks for the review. yes it can improve the error to the user in ovs-vsctl
> show.
>
> error: "enp8s0f1: could not set configuration (Invalid
> argument)"
> vs
>
> error: "enp8s0f1: Cannot get flow control parameters: Invalid
> argument"
>
>
> I'll do it in a different commit as its not related to the memleak fix.
Thanks,
I have gone ahead and applied this patch to main.
- netdev-dpdk: Fix possible memory leak configuring VF MAC address.
https://github.com/openvswitch/ovs/commit/4f29804f249b
As a follow-up I will prepare backports back to v2.17.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
