Re: [PATCH] (sqlbox) sqlbox.patch

Mi Reflejo wrote:

> New sqlbox.patch for sqlbox module according to the last gateway update.

Hi Martin,

thanks a lot for the submission. We appritiate your work on Kannel, and I have 
to admit that this is a good way to go...

Now, some critics anyway (even while Rene is more in charge for sqlbox ;)

* Please don't include configure itself in the patchset. It's "generated" code 
and hence, only configure.in belongs to the patchset. (It's like if we would add 
.o files to the CVS ;)

* The Makefile.in change is necessary to let Kannel core build sqlbox. This can 
be moved out of Kannel core in order to allow sqlbox (as own add-on module) to 
build itself again Kannel core (gwlib, gw) libs... I could forward you my add-on 
   sceleton configure.in and Makefile.in to use for such purposes. You could go 
then and addopt it for sqlbox?

* Why is
diff -u -r1.166 configure.in
--- configure.in.orig   2006-06-13 15:27:52.000000000 -0600
+++ configure.in        2006-06-13 15:27:15.000000000 -0600
@@ -936,14 +936,14 @@
          dnl mysql-4.x style
          MYSQL_LIBS=""
          if $MYSQL_CONFIG --libs_r &>/dev/null ; then
-            MYSQL_LIBS=`$MYSQL_CONFIG --libs_r`
+            MYSQL_LIBS=`$MYSQL_CONFIG --libs_r | sed -e "s/\x27//g"`
              AC_MSG_CHECKING([mysql reentrant libs])
              AC_MSG_RESULT([$MYSQL_LIBS])
              AC_CHECK_LIB(mysqlclient_r, mysql_init, [ LIBS="$LIBS $MYSQL_LIBS" ],
                  [ MYSQL_LIBS="" ], [ $MYSQL_LIBS ])
          fi
          if test -z "$MYSQL_LIBS" ; then
-            MYSQL_LIBS=`$MYSQL_CONFIG --libs`
+            MYSQL_LIBS=`$MYSQL_CONFIG --libs | sed -e "s/\x27//g"`
              AC_MSG_CHECKING([mysql libs])
              AC_MSG_RESULT([$MYSQL_LIBS])
              AC_CHECK_LIB(mysqlclient, mysql_init, [ LIBS="$LIBS $MYSQL_LIBS" ],

necessary? Was this the issue with the ' signs?
If yes, this is of general scope and should go to gateway core itself.

* Why is config directive 'bearerbox-is-sqlbox' necessary for smsbox.c? I don't 
see a usage of the added int bearerbox_is_sqlbox in the smsbox.c patchset?

Does this mean sqlbox acts towards a "patched" smsbox like a bearerbox? Like this:

   user <-HTTP-> smsbox <-> sqlbox <-> bearerbox

?

* sqlbox should have it's own module gwlib/sqlbox-cfg.def and call the 
cfg_add_hooks() routine I proposed to "load" the semantic checks for the new 
config group.


Hope I wasn't to critic ;)... actually I like the sqlbox itself, but would 
suggest Rene and you to mangle it the way I used for smppbox, where the module 
should be as much isolated from patching the core as possible. In the end I came 
up with the single (still to vote) patch for the cfg_add_hook() patch for 
gwlib/cfg.[ch], and everything else would be in foobar module dir itself.... 
works perfectly and smooth.

Stipe

-------------------------------------------------------------------
Kölner Landstrasse 419
40589 Düsseldorf, NRW, Germany

tolj.org system architecture      Kannel Software Foundation (KSF)
http://www.tolj.org/              http://www.kannel.org/

mailto:st_{at}_tolj.org           mailto:stolj_{at}_kannel.org
-------------------------------------------------------------------