> On 21 Sep 2017, at 14:58, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>>>
>>> On 20 Sep 2017, at 18:27, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>>> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel