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

[Christophe de Dinechin]

> On 21 Sep 2017, at 14:58, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 
>>> 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.

If Christophe F feels comfortable with that and if others do not object, go for it.

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