On Tue, Apr 23, 2024 at 11:33:15AM +0100, Simon Horman wrote:
> 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.
I now have applied the following backports.
Backport applied to branch-3.3:
- netdev-dpdk: Fix possible memory leak configuring VF MAC address.
https://github.com/openvswitch/ovs/commit/a6c3b5202c2f
Backport applied to branch-3.2:
- netdev-dpdk: Fix possible memory leak configuring VF MAC address.
https://github.com/openvswitch/ovs/commit/4e4463ca5539
Backport applied to branch-3.1:
- netdev-dpdk: Fix possible memory leak configuring VF MAC address.
https://github.com/openvswitch/ovs/commit/6ce0130f1239
Backport applied to branch-3.0:
- netdev-dpdk: Fix possible memory leak configuring VF MAC address.
https://github.com/openvswitch/ovs/commit/e5f237e9a95d
Backport applied to branch-2.17:
- netdev-dpdk: Fix possible memory leak configuring VF MAC address.
https://github.com/openvswitch/ovs/commit/b2e19110394e
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
