> 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. 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. > >> >> Anyway, if we were starting to write the code, I’d agree. But here, we do a big patch >> just to remove the *existing* wrapper, including changes at practically all call sites. >> That means a future rebase is going to show potential conflicts all over the place. >> For everybody with branches that touch anything close to one of these call sites. >> >> So the question is: how does putting a macro wrapper not solve the >> alleged problem with having a wrapper, whatever that problem is, without >> requiring all call sites to be modified? > > I don't understand what you are asking in the last paragraph. If you are > having an issue with all the churn this patch series causes (the rebase > problem you mention in the paragraph before the last), that's a fair > point, and I agree that's a concern. But I don't think this is what you > were discussing before this email. 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. Christophe _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel