Re: new rrd tool extension

On Mon, 2010-05-10 at 18:23 +0200, Miroslav Kubelík - seznam wrote:
> Hello,
> I made first production ready code for a new php extension for a rrd tool  
> (http://oss.oetiker.ch/rrdtool/). I think rrd tool is widely know. There  
> is already current php extension in rrd tool svn, but is very ugly from my  
> point of view (old PHP4 1:1 wrapper around C API with very ugly function  
> parameters).
> 
> My extension - http://hg.mirin.cz/phprrd/file/tip - has a new OO API.  
> Currently create, update, graph functionality are supported. It's enough  
> for most of rrd use cases, maybe I'll add support for "info" and "multiple  
> timestamps update".
> 
> Extension license will be PHP.
> 
> What do you think about maintaining in pecl for next development and  
> improvements?

Quick review looks fine, a few small comments though:

      * rrdtool uses GPL with FLOSS Exception. Exception is valid for
        PHp License 3.0, not the current version 3.01, could you talk to
        the rrdtool maintainers to add version 3.01? (should be straight
        forward, but better to be on the safe side legal-wise) Mind the
        small difference in paragraph 6 between these two:
        http://www.php.net/license/3_0.txt
        http://www.php.net/license/3_01.txt
      * in rrd.c you're using C99/C++ comments, please use always /* */
        instead of //. 
      * when using zend_parse_parameters with "s" modifier the length is
        int not long.
      * In rrd_create_add_string_arg() and other places you're iterating
        over HashTables. You assume the consist of zvals of string type
        only, are you certain no Z_TYPE_PP(item) == IS_STIRNG checks or
        similar are needed?
      * In rrd_update.c you're doing emalloc(argc * sizeof(char *));
        it's better to use safe_emalloc in such cases.

All small things, looks good overall.

johannes