> On 20 Sep 2017, at 18:27, Christophe Fergeau <cferg...@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 <cferg...@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 <cferg...@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
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
