[Spice-devel] [PATCH spice-server v2 00/23] Use GLib memory allocation

[Christophe Fergeau]

On Wed, Sep 20, 2017 at 06:15:49PM +0200, Christophe de Dinechin wrote:
> 
> 
> > On 20 Sep 2017, at 17:51, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > On Wed, Sep 20, 2017 at 05:31:37PM +0200, Christophe de Dinechin wrote:
> >> 
> >> 
> >>> On 20 Sep 2017, at 16:51, Christophe Fergeau <cfergeau at redhat.com> wrote:
> >>> 
> >>> On Wed, Sep 20, 2017 at 02:54:31PM +0200, Christophe de Dinechin wrote:
> >>>>> 
> >>>>>> The benefit of doing it that way (in addition to requiring less source code
> >>>>>> changes and making following rebases or merge much easier) is that it leaves
> >>>>>> the option to instrument spice allocations specifically when the need
> >>>>>> arises.
> >>>>>> 
> >>>>> 
> >>>>> There are many tools to instruments memory allocations and is not hard
> >>>>> to write one on your own. For instance knowing that objects file takes
> >>>>> precedence over libraries you can write a module defining malloc, or use
> >>>>> --wrap linker option or LD_PRELOAD.
> >>>> 
> >>>> That works if you want to instrument all malloc calls. If you want to do
> >>>> something specific to spice, you can’t do that.
> >>> 
> >>> You could do that with systemtap for example. And I really don't think
> >>> we should have our spice_xxx wrappers for library calls.
> >> 
> >> But then, we don’t need g_xxx wrappers either, do we?
> > 
> > They (both g_malloc and spice_malloc) abort on OOM, straight malloc does not.
> 
> I know. And if that’s the only value add now, it does not mean it will always be like that.
> Having the wrapper means we can do something special for spice allocations, if only
> change the error message or display some spice context for the error. Just because
> we don’t do this today does not mean it’s a good idea to make it impossible in the future.

Note that in 6+ years, we haven't done that :)

> 
> If the problem is that some places use some other glib wrapper as Frediano suggested, then
> let’s convert these other places to use spice_malloc rather than the other way round.
> 
> I know that there has been a trend to “de-spicify” and “glibify” things recently.
> I frankly don’t understand that trend. To me, that’s running a bit backwards.

In the past, spice-server did not have a glib dependency, so it added
all sort of useful wrappers quite similar to glib. Now that we have a
glib dependency, keeping them around just seems pointless to me,
confusing to newcomers ("there must be a difference between the glib
function and the spice one!!").

> 
> > 
> I think it always was. Quoting my first response:
> 
> > I am a bit ambivalent about this kind of source-level replacement. Why not simply #define spice_malloc to glib_malloc?
> > 
> > The benefit of doing it that way (in addition to requiring less source code changes and making following rebases or merge much easier) is that it leaves the option to instrument spice allocations specifically when the need arises.
> 
> Let me stress “In addition to requiring less source code change and making following rebases or merge much easier”. I was really talking about that from the very beginning.

I read it as "let's do s/free/spice_free/, one nice side-effect is that
it means less code changes/rebase issues/...", rather than "I'm really
concerned about the impact on rebases these patches are going to have".
In the latter case, one possible option is to drop the patch series, in
the former case, the suggestion is to change it :)

Christophe