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@xxxxxxxxx>
> > ---
> > 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.
