Re: [PATCH 03/28] domain_conf: use g_free() in virDomainPostParseCheckISCSIPath()
On Mon, Nov 09, 2020 at 15:37:39 +0100, Michal Privoznik wrote:
On 11/6/20 4:32 AM, Matt Coleman wrote:
> Signed-off-by: Matt Coleman <matt(a)datto.com>
> ---
> src/conf/domain_conf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ce49905360..a64dec8df4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5078,7 +5078,7 @@ virDomainPostParseCheckISCSIPath(char **srcpath)
> return;
> path = g_strdup_printf("%s/0", *srcpath);
> - VIR_FREE(*srcpath);
> + g_free(*srcpath);
> *srcpath = g_steal_pointer(&path);
> }
>
Can't we do this for other places too? I mean, this boils down to
discussions we had when starting to adopt glib, but rather than doing this
change per function I think we want bigger blocks. The same applies for
20/28 where you're switching to g_renew() from VIR_REALLOC_N(). I understand
that you want to touch only some functions because later you are turning
their return type into void, but I'd rather see bulk glib conversions.
Specifically you can only replace VIR_FREE with g_free if you
immediately overwrite it.
Our semantics of VIR_FREE lead in many cases to a code pattern of using
VIR_FREE and then reusing the variable and having another VIR_FREE in
the cleanup path. Since g_free doesn't clear the pointer this would lead
to double frees when blindly replaced.
Non-blind replace is obviously very expensive both for the author and
for the reviewer.
The only blind replacement we can do is to replace VIR_FREE with
g_clear_pointer(&ptr, g_free);
This is in the end what VIR_FREE exactly does.
Now it's only necessary to justify the churn. To me it's borderline not
worth it.