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

[Benny Baumann]

Hi,

I had a look at your modified version and am not quiet lucky with it.

For the format: Trimming trailing whitespace at EOL isn't that hard, but
that's more or less a format issue.

More problematic are some memory leaks you introduced that cause memory
to stay allocated till termination of a request unnecessarily if a
RRDTool function fails. Furthermore you free argv in rrd_restore which
will cause severe issues.

Using the zend_parse_param API shoud be more or less used consistently
IMHO. Some functions use it; some don't. IMHO either all functions
should (including checks for number of parameters) or none should. For
obvious reasons I'd prefer the first version ...

Another thing I'm not happy with yet is the handling of NaNs. Shouldn't
the PHP Extension return the NaN values as NaNs instead of strings? I'd
think so. Short note on how to return them as NaNs would be nice (or
simply return the NaN as passed in by librrd?).

The "estrdup all arguments into an array" should go to a central
function IMHO:
char **__rrd_argument_strdup(char *op, char *preceed, int preceed_len,
HashTable *args) {
    HashPosition pointer;
    zval **data;

    argc = zend_hash_num_elements(args) + 2;
    if(preceed && preceed_len)
        argc++;

    argv = (char **) emalloc(argc * sizeof(char *));

    argv[0] = "dummy";
    argv[1] = estrdup(op);

    i = 2;

    if(preceed && preceed_len)
        argv[i++] = estrndup(preceed,preceed_len);

    for(zend_hash_internal_pointer_reset_ex(args, &pointer);
        zend_hash_get_current_data_ex(args, (void**) &data, &pointer) ==
SUCCESS;
        zend_hash_move_forward_ex(args, &pointer)) {

        zval temp = **data;

        zval_copy_ctor(&temp);
        convert_to_string(&temp);

        argv[i++] = estrdup(Z_STRVAL(temp));

        zval_dtor(&temp);
    }

    return argv;
}

Why are some parts of the source programmed binary-safe (e.g. the
estrndup of the filename), while neither rrdtool (librrd) nor
php_check_open_basedir are binary-safe? Shouldn't functions better fail
on non-binary-safe input (i.e. contained \0-bytes)?

@rrd_info: Although its nice to have the array parsing my demo script
showed done inside the extension, it shouldn't use static buffers for
parsing (even if you assume Tobi has no malicious intent ;-)) Not only
are they wasting stack space, but they also need to duplicate the
strings they work on. The better alternative IMHO would be to do the
parsing inside the string (key) returned by rrd_info lookup the array
the value should go to (and if necessary create it) or fallback to flat
arrays if the key can't be parsed. That way you won't need to hardcode
certain patterns but can use the flexible key syntax keynames already
use. The reason why I didn't do this in my userland wrapper simply was
lazyness ;-) But in the extension it should be done properly if its done.

A note BTW: Your source doesn't check for availability of rrd_dump_cb_r
... well, mine didn't neither, so just a note on this ;-)

Short Question: Why did you remove the COMPILE_DL_RRDTOOL checks?

So much for now. I'll integrate the mentioned changes ASAP and will
release a new preview once I'm done.

Regards,
BenBE.

P.S.: @Tobi: Do AND did all versions of librrd initialize opt_ind and
opt_err themselves or did some versions of librrd rely on the user doing
so? If some didn't: Which ones?

Am 25.08.2009 22:20, schrieb Dan Cech:
> Benny et al,
>
> I have attached my current working version which includes many of the
> updates previously posted by Benny in addition to a number of the
> updates I have been working on.
>
> This includes:
>
> - remove requirement to send argument count to rrd_graph
> - add support for rrd_xport
> - remove requirement to send argument count to rrd_fetch
> - support multiple update values in rrd_update
> - support rrd_lastupdate
> - support rrd_first
> - support rrd_info
>
> As always, all feedback is appreciated.
>
> Dan
>


-------------- 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/4264e8a7/attachment.pgp