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

[Christophe de Dinechin]

> On 20 Sep 2017, at 11:53, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 
>>> On 20 Sep 2017, at 09:50, Frediano Ziglio <fziglio at redhat.com> wrote:
>>> 
>>> Reduce the usage of spice_new*/spice_malloc* allocations.
>>> They were designed in a similar way to GLib ones.
>>> Now that we use GLib make sense to remove them.
>>> However the versions we support for GLib can use different memory
>>> allocators so we have to match g_free with GLib allocations
>>> and spice_* ones (which uses always malloc allocator) with free().
>>> 
>>> The final target is to remove spice_new*/spice_malloc*
>>> functions/helper from spice-common.
>> 
>> I am a bit ambivalent about this kind of source-level replacement. Why not
>> simply #define spice_malloc to glib_malloc?
>> 
> 
> Does not work as you need to change free according where needed.

But you need to change free calls in your patch too, right?

> Unless you add a spice_free too (and change required free calls).

Well, if we change things, I’d rather change them in that direction, yes.

> But the spice_XXX functions were designed like GLib so would cause some
> confusion having 2 sets of functions potentially using different pools.

This is a problem you already need to address in your patch, if I’m not mistaken.

> 
>> 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.

> 
>>> 
>>> This series finish changing all allocations from spice_XXX
>>> to Glib memory functions (tests excluded).
>>> 
>>> You will notice some patches have changes only in allocation part
>>> without any free. This means code has leaks, not a regression.
>>> 
>>> If you check the code there is still a spice_malloc_n call in
>>> display-channel.c. This is required as the memory is freed by
>>> spice-common with a normal free.
>>> 
>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>> 
>>> Frediano Ziglio (23):
>>> lz4-encoder: Use GLib memory functions
>>> mjpeg: Use GLib memory functions
>>> reds-stream: Use GLib memory functions
>>> smartcard: Use GLib memory functions
>>> stream: Use GLib memory functions
>>> spicevmc: Use GLib memory functions
>>> gstreamer-encoder: Use GLib memory functions
>>> char-device: Use GLib memory functions
>>> event-loop: Use GLib memory functions
>>> dispatcher: Use GLib memory functions
>>> Use GLib memory functions for SpiceChannelEventInfo
>>> reds: Use GLib memory functions for link message
>>> reds: Use GLib memory functions for ChannelSecurityOptions
>>> dcc: Use GLib memory functions
>>> display-channel: Use GLib memory functions
>>> replay-qxl: Use GLib memory functions
>>> worker: Use GLib memory functions
>>> tree: Use GLib memory functions
>>> inputs-channel: Use GLib memory functions
>>> image-cache: Use GLib memory functions
>>> pixmap-cache: Use GLib memory functions
>>> parse-qxl: Use GLib memory functions
>>> red-pipe-item: Use GLib memory functions
>>> 
>>> server/cache-item.tmpl.c          |  6 ++---
>>> server/char-device.c              | 12 +++++-----
>>> server/cursor-channel.c           |  2 +-
>>> server/dcc.c                      | 40 ++++++++++++++++----------------
>>> server/dispatcher.c               |  4 ++--
>>> server/display-channel.c          |  6 ++---
>>> server/event-loop.c               |  8 +++----
>>> server/gstreamer-encoder.c        | 16 ++++++-------
>>> server/image-cache.c              |  4 ++--
>>> server/inputs-channel.c           | 10 ++++----
>>> server/lz4-encoder.c              |  8 +++----
>>> server/main-channel-client.c      | 14 ++++++------
>>> server/mjpeg-encoder.c            | 28 +++++++++++------------
>>> server/pixmap-cache.c             |  6 ++---
>>> server/red-channel-client.c       |  8 +++----
>>> server/red-channel.c              |  2 +-
>>> server/red-parse-qxl.c            | 46
>>> ++++++++++++++++++-------------------
>>> server/red-pipe-item.c            |  2 +-
>>> server/red-replay-qxl.c           | 48
>>> +++++++++++++++++++--------------------
>>> server/red-worker.c               |  8 +++----
>>> server/reds-stream.c              | 39 ++++++++++++-------------------
>>> server/reds.c                     | 14 ++++++------
>>> server/smartcard-channel-client.c |  2 +-
>>> server/smartcard.c                | 14 ++++++------
>>> server/spicevmc.c                 | 24 ++++++++++----------
>>> server/stream.c                   |  8 +++----
>>> server/tree.c                     |  8 +++----
>>> 27 files changed, 189 insertions(+), 198 deletions(-)