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;
}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
