Hello Gaurav, On Tue, Sep 22, 2020 at 12:27 PM Power, Ciara <[email protected]> wrote: > >-----Original Message----- > >From: dev <[email protected]> On Behalf Of Honnappa Nagarahalli > >Sent: Tuesday 11 August 2020 22:01 > >To: Gaurav Singh <[email protected]>; [email protected] > >Cc: nd <[email protected]>; Honnappa Nagarahalli > ><[email protected]>; nd <[email protected]> > >Subject: Re: [dpdk-dev] [PATCH] lib/metrics: fix memory leak > > > ><snip> > > > >> > >> fix memory leak > >> > >> Fixes: c5b7197f66 ("telemetry: move some functions to metrics > >> library") > >> > >> Signed-off-by: Gaurav Singh <[email protected]> > >> --- > >> lib/librte_metrics/rte_metrics_telemetry.c | 21 ++++++++++++++++----- > >> 1 file changed, 16 insertions(+), 5 deletions(-) > > > I think this commit message should be more descriptive, and is missing Cc: > [email protected] > > > >> diff --git a/lib/librte_metrics/rte_metrics_telemetry.c > >> b/lib/librte_metrics/rte_metrics_telemetry.c > >> index 289ebae0b..7b6d1063c 100644 > >> --- a/lib/librte_metrics/rte_metrics_telemetry.c > >> +++ b/lib/librte_metrics/rte_metrics_telemetry.c > >> @@ -41,12 +41,17 @@ > >> rte_metrics_tel_reg_port_ethdev_to_metrics(uint16_t > >> port_id) > >> } > >> > >> xstats_names = malloc(sizeof(*xstats_names) * num_xstats); > >> + if (xstats_names == NULL) { > >> + METRICS_LOG_ERR("Failed to malloc memory for > >> xstats_names"); > >> + return -ENOMEM; > >> + } > >> + > >> eth_xstats_names = malloc(sizeof(struct rte_eth_xstat_name) > >> * num_xstats); > >> - if (eth_xstats_names == NULL || xstats_names == NULL) { > >> - METRICS_LOG_ERR("Failed to malloc memory for > >> xstats_names"); > >> - ret = -ENOMEM; > >> - goto free_xstats; > >> + if (eth_xstats_names == NULL) { > >> + METRICS_LOG_ERR("Failed to malloc memory for > >> eth_xstats_names"); > >> + free(xstats_names); > >> + return -ENOMEM; > >> } > > Is there a reason for the above changes? I think they are unrelated to > the memory leak this patch is fixing. > > >> if (rte_eth_xstats_get_names(port_id, > >> @@ -167,9 +172,15 @@ rte_metrics_tel_format_port(uint32_t pid, json_t > >> *ports, > >> } > >> > >> metrics = malloc(sizeof(struct rte_metric_value) * num_metrics); > >> + if (metrics == NULL) { > >> + METRICS_LOG_ERR("Cannot allocate memory"); > >> + return -ENOMEM; > >> + } > >> + > >> names = malloc(sizeof(struct rte_metric_name) * num_metrics); > >> - if (metrics == NULL || names == NULL) { > >> + if (names == NULL) { > >> METRICS_LOG_ERR("Cannot allocate memory"); > >> + free(metrics); > >> return -ENOMEM; > >> } > > This does fix the resource leak, but I do think it can be done in a > simpler way, as shown in the patch I sent to fix the logged coverity issue > for this: https://patchwork.dpdk.org/patch/78052/ > I will remove my patch seeing as this patch is fixing the same thing.
I agree with Ciara. Could you respin? Thanks. -- David Marchand
