[rrd-developers] Preview for updated RRDTool PHP extension and userland wrapper

[Benny Baumann]

Hi,
> I like the work you did moving out the args array creation, good idea.
>   
Thanks. I just saw that most routines look simular so I optimized this.

IMPORTANT: Please note that I missed to fix rrd_update (the emalloc
there should be sizeof(rrd_...) without the additional star.
> I'm confused about the purpose of setting args[0] = "dummy", is there a
> reason behind this or could we simplify things by removing it?
>   
That was from the original source. I believe it is done this way to get
the argument indices like in normal shell scripts on the API side, but
I'm not sure about this. If you like you can send me a patch to use
zero-based indices and I'll integrate it.
> Regarding the argument checks, I don't see any compelling reason to
> break compatibility with old code which passes the argument count as the
> 3rd parameter.
>   
Thie latest version does a dummy read for the argc as a optional long
parameter which gets ignored without any further notice.
> This is why my patch separated the argument count check from the
> zend_parse_parameters call, otherwise the result was the
> WRONG_PARAM_COUNT error.  For newly-added functions there is obviously
> no need to support any old usage.
>   
Yes there isn't, though "emulating" it makes the interface more
consistent IMHO. And since doing the dummy argc with
zend_parse_parameters is only little changes I included it for all the
sa|l-style functions.

For rrd_xport: where do you pass the file to xport from? Shouldn't this
be the first parameter? Or is the filename passed in as part of the args
array in some "encoded" form like with rrd_graph?
> Basically my thinking is that there should be no reason that this new
> version won't be able to work with existing php code built against the
> old version, which will make things much easier from a maintenance
> perspective in that users don't have to care whether upgrading to the
> new version of the extension will break existing php code.
>   
k.
> Those who are better versed in how this will affect getting the new
> version packaged for debian, etc can probably give more insight on this
> point.
>   
I hope Stefan who takes care of the overall Debian Packaging of RRDTool
could do a php-rrdtool package too if it isn't too much work.
>> Now it uses the internal array copy routine or builds the necessary
>> structure for strings. Makes source more readable and reduces branches.
>>     
> Looks fine to me.
>   
Please note the emalloc Heap Overflow is still in rrd_dump for zpp("ss")
- I'll release another patched version once there has been some more
changes accumulated.
>>> - support rrd_lastupdate
>>>       
>> Didn't compile for me. Had to change for rrd_lastupdate_r to make it
>> work (perhaps an error in rrd.h???)
>>     
> Hmm, was working for me.  No matter using rrd_lastupdate_r is neater.
>   
Yep. And threadsafe ;-) I use the 1.3.999 headers (packaged as deb file)
on my box.
>>> - support rrd_first
>>>       
>> Reworked argument parsing and preparation
>>     
>>> - support rrd_info
>>>       
>> Reworked argument parsing and preparation
>>     
> Looks like we're making some headway!
>   
Seems so.

Do you have any experience with the rrd cache daemon and the API? Would
be nice if we could add support for this too. I have none experience
with that part of the API yet as I didn't need the daemon yet.
> Dan
Regards,
BenBE.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
Url : http://lists.oetiker.ch/pipermail/rrd-developers/attachments/20090826/be121639/attachment-0001.pgp