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