HI Harald,
On Tue, Sep 8, 2015 at 10:13 AM, Harald Welte laforge@gnumonks.org wrote:
On Mon, Sep 07, 2015 at 01:08:47AM -0500, Alexander Chemeris wrote:
- a so-called 'fix for use after free' that is actually a patch that introduces another copy for every primitive and is only required for the loopback mode
I'd appreciate recommendations on how to do this differently. IIRC the code frees messages after the function and the queue was pointing to a freed message. this led to undefined behavior.
There is a companion patch to this to manually activate/deactivate a channel. I'd appreciate recommendations on how to properly implement it as well. Loopback and channel activation functions are very helpful for the L0/L1 development.
I don't really have a good response for this, other than to keep it out of master (or maybe even a compile time option). 99.9% of all installations will not have any benefit from the extra memcpy(), so I don't want to make it the standard behavior.
I took a look into this patch today.
So, first of all - as I expected, it changes behavior only for channels marked as loopback. I.e. it has no effect on production systems.
But nevertheless, I found a cleaner solution and checked it in as two first commits at the 201509-fairwaves-rebase branch. If we return "1" from the function, the message is not being freed and we don't need to create a copy of it. I think it worth merging, as it doesn't degrade anything and is a clean fix.
Could you share what kind of testing has been performed, so we can shape our expectations?
Not much, to be honest. We will test for osmo-bts-sysmo during this week. However, no testing will be done by me regarding the osmo-trx related code.
Ok, noted.
I tried to run the code today, but was not able to get osmo-bts to start at all. It stop with an error: <0011> e1_input.c:326 No such E1 driver 'ipa Have you seen this? I guess this comes from the common part of the code, but I don't have much experience with this part of the code, unfortunately.