[libvirt] [PATCH 7/7] util: use glib allocation functions

[Daniel P. Berrangé]

On Mon, Sep 02, 2019 at 04:52:47PM +0200, Ján Tomko wrote:
> On Thu, Aug 29, 2019 at 07:02:50PM +0100, Daniel P. Berrangé wrote:
> > GLib requires that any memory allocated with g_new/g_malloc/etc
> > is free'd by g_free. This means mixing virAlloc with g_free,
> > or g_new with virFree will violate API rules. The easy way to
> > dea with this is to simply make our allocation functions wrappers
> 
> s/dea/deal/
> 
> > to the glib functions.
> > 
> > Use of our allocation functions can be phased out gradually
> > over time. When doing such conversions, the return values checks
> > can be dropped at the same time.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> > src/util/viralloc.c | 29 ++++++-----------------------
> > 1 file changed, 6 insertions(+), 23 deletions(-)
> > 
> 
> I'm afraid this is incomplete.
> 
> The obvious mismatch being VIR_STRDUP calling strdup while
> VIR_FREE would call g_free now.

Opps, yes.

> Same with (v)asprintf. We also use VIR_FREE to free memory
> allocated by external libraries (e.g. virXMLPropString; while
> there is a xmlFree function which is just an alias for free,
> we never use it in libvirt code)

Hmm, not using xmlFree is arguably a bug in libvirt code.

Most other libs quite probably mandate plain 'free'.

> Maybe the safer approach would be to convert the code gradually
> and explicitly use g_alloc and g_free/g_autoptr without the wrappers?

I'm not convince that would be especially safe - the caller of any
API which returns an allocated string now checks to look at the impl
to see how it was allocated - with deep call chains this gets quite
error prone I think.

What's interesting is that the glib docs says

 * It's important to match g_malloc() (and wrappers such as g_new()) with
 * g_free(), g_slice_alloc() (and wrappers such as g_slice_new()) with
 * g_slice_free(), plain malloc() with free(), and (if you're using C++)
 * new with delete and new[] with delete[]. Otherwise bad things can happen,
 * since these allocators may use different memory pools (and new/delete call
 * constructors and destructors).

but the actual code just does

gpointer
g_malloc (gsize n_bytes)
{
  if (G_LIKELY (n_bytes))
    {
      gpointer mem;

      mem = malloc (n_bytes);
      TRACE (GLIB_MEM_ALLOC((void*) mem, (unsigned int) n_bytes, 0, 0));
      if (mem)
        return mem;

      g_error ("%s: failed to allocate %"G_GSIZE_FORMAT" bytes",
               G_STRLOC, n_bytes);
    }

  TRACE(GLIB_MEM_ALLOC((void*) NULL, (int) n_bytes, 0, 0));

  return NULL;
}

void
g_free (gpointer mem)
{
  free (mem);
  TRACE(GLIB_MEM_FREE((void*) mem));
}

IOW, it is entirely safe to mix free + g_free today - looks like the warning
is just talking about a hypothetical risk that doesn't actually exist right
now, at least for free vs g_free.

The g_slice warning is definitely relevant though since that uses a pool
allocator

So I'm inclined to fix the mistakes in this patch, and then just deal with
using plain 'free' for any cases we deal with external libraries on best
effort, in the belief that it doesnt' actually matter for glib anyway.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|