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

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(a)redhat.com&gt; > > --- > > 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.