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

[Frediano Ziglio]

> 
> > On 20 Sep 2017, at 18:27, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > 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 :)
> 
> If that was not clear, my suggestion was rather to change it.
> 
> 
> > Christophe

I think this is a matter of style and as such just different opinions.

Considering the current mixed style I would proposed a poll.

I obviously vote for moving to GLib functions directly.

I hope we don't have to vote for having a coherent style in the code.

Frediano