Re: [libvirt] [PATCH 6/9] util: Refactor 'virResctrlMonitorFreeStats'
On 2019年05月27日 23:26, Michal Privoznik wrote:
On 5/23/19 11:34 AM, Wang Huaqiang wrote:
> Refactor 'virResctrlMonitorFreeStats' to let it available to free
> the 'virResctrlMonitorStatsPtr' variable.
>
> Signed-off-by: Wang Huaqiang <huaqiang.wang(a)intel.com>
> ---
> src/qemu/qemu_driver.c | 1 +
> src/util/virresctrl.c | 4 +---
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 42b1ce2..2abed86 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19980,6 +19980,7 @@
> qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
> VIR_FREE(resdata->name);
> VIR_FREE(resdata->vcpus);
> virResctrlMonitorFreeStats(resdata->stats, resdata->nstats);
> + VIR_FREE(resdata->stats);
> VIR_FREE(resdata);
> }
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 90532cf..0f18d2b 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -2764,7 +2764,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr
> monitor,
> cleanup:
> VIR_FREE(datapath);
> VIR_FREE(filepath);
> - VIR_FREE(stat);
> + virResctrlMonitorFreeStats(&stat, 1);
How about creating a function that frees exactly one
virResctrlMonitorStatsPtr and then (possibly) have a wrapper that
would call it 1 to n times? We can then get rid of this ugly call.
How about change virResctrlMonitorFreeStats to: (function name have been
changed to
virResctrlMonitorStatsFree)
```code
void
virResctrlMonitorStatsFree(virResctrlMonitorStatsPtr stat)
{
if (!stat)
return;
VIR_FREE(stat->vals);
virStringListFree(stat->features);
VIR_FREE(stat); /* Free virResctrlMonitorStats object */
}
```
And following code to is the way to free a list of
@virResctrlMonitorStatsPtr objects:
```code
static void
qemuDomainFreeResctrlMonData(virQEMUResctrlMonDataPtr resdata)
{
...
size_t i = 0;
for ( i = 0; i < resdata->nstats; i++)
virResctrlMonitorStatsFree(resdata->stats[i]);
virResctrlMonitorStatsFree
...
}
```
> VIR_DIR_CLOSE(dirp);
> return ret;
> }
> @@ -2781,8 +2781,6 @@
> virResctrlMonitorFreeStats(virResctrlMonitorStatsPtr *stats,
> for (i = 0; i < nstats; i++)
> VIR_FREE(stats[i]);
> -
> - VIR_FREE(stats);
This means that virResctrlMonitorGetStats() is now leaking @stat. For
instance, if virStrToLong_uip() fails.
Got. Thanks.
Michal
Thanks for review.
Huaqiang