Re: [PATCH 03/28] domain_conf: use g_free() in virDomainPostParseCheckISCSIPath()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux