[sr-dev] [SR-Users] Freezing development for Kamailio v4.1.0

Timo Teras timo.teras at iki.fi
Wed Oct 2 15:26:57 CEST 2013


On Wed, 02 Oct 2013 15:20:33 +0200
Daniel-Constantin Mierla <miconda at gmail.com> wrote:

> 
> On 10/2/13 8:25 AM, Timo Teras wrote:
> > On Mon, 30 Sep 2013 14:46:48 +0200
> > "Olle E. Johansson" <oej at edvina.net> wrote:
> >
> >> 30 sep 2013 kl. 14:04 skrev Daniel-Constantin Mierla
> >> <miconda at gmail.com>:
> >>
> >>> Hello,
> >>>
> >>> On 9/30/13 12:08 PM, Timo Teras wrote:
> >>>> On Mon, 30 Sep 2013 11:48:10 +0200
> >>>> Daniel-Constantin Mierla <miconda at gmail.com> wrote:
> >>>>
> >>>>> a short reminder that we are one week before feature freeze for
> >>>>> next major release.
> >>>> I would like to incorporate the module from:
> >>>> https://github.com/rdboisvert/mohqueue
> >>>>
> >>>> Could you review it?
> >>>>
> >>>> I am willing to be maintainer for it if needed.
> >>> interesting feature - no need for special review because it is a
> >>> stand alone module. You are already registered developer, so you
> >>> can review yourself a bit if you didn't do (use) it already - the
> >>> review of new modules from new developers is to check quickly if
> >>> they use internal memory managers, DB API and/or spot if something
> >>> else from existing code can be reused.
> >> Agree, this is a really cool feature. I can think of a ton of RPC
> >> commands as well as stats/counters I would like to see. We're
> >> looking forward to this!
> > The author asked me to do the initial merge. He might be willing to
> > take over the maintainer ship in few weeks, and we can come back
> > to this discussion after a bit.
> >
> > I created 'tteras/mohqueue' branch and pushed the module there
> > as-is - with LICENSE removed as being redundant now that it is part
> > of the main tree.
> >
> > If possible please take a look at it and post any comments to fix
> > before merge to master, which I plan to do tomorrow.
> >
> > Robert, I also saw that you had created new branch 'noRTPstop' with
> > three additional commits - please let me know if those are to be
> > included too. Let me know also if there's any additional changes
> > you'd like to have in.
> The code indentation style is hard to follow for me, because the
> blocks are not easy visible (for what I used to, probably). I would
> prefer a style using tabs, with blocks indenting the inner acctions
> (e.g., like in the core and most of the modules, that should be
> pretty much Allman or K&R). Anyhow, it is not a must as long as
> developer of that code keeps maintaining the code and doesn't need
> help from others.

I had the same comment. I'll just run it through indent.

>  From a quick look at main file in the module:
> - fixup_str() can be replaced with fixup_spve_spve(...) - it is a 
> function that allows static and dynamic string parameters
> - fixup_count() is then fixup_spve_null(...)

Ok, good points.

> The sql tables definition have to be made in xml and added to 
> lib/srdb1/schema/, then the sql script will be generated for all db 
> connectors via:
> 
> make dbschema
> 
> So modules/mohqueue/mohqueue.sql should not be part of the module --
> sql scripts will be part of utils/kamctl/[mysql|postgres|...]/.

Oh right, forgot this.

I also noted that the locking could be simplified, and use rwlock
primites instead of rewriting it as spinlocks.

-Timo




More information about the sr-dev mailing list