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