Hi Jérôme, thank you very much for your patch :)
I have a couple of small suggestions / questios regarding the code,
maybe you could help me out:
* You forgot to add yourself as copyright holder at the beginning of the
file. I guess you copied the "apache" plugin and modified it. Since
there's basically nothing of the "apache" plugin left, I'd suggest you
simply remove those other lines.
* The new "types" introduces by the patch, "varnish_cache" and
"varnish_connections", have multiple "data sources" each. This is
deprecated and only used when there's a good reason for it. I think
the "cache_ratio" type could be used instead of "varnish_cache". Many
other plugins define their own "foo_connections" type, for example
"memcached_connections" and "nginx_connections". Feel free to copy one
of those definitions to "varnish_connections". [*]
* The only argument passed to "VSL_OpenStats" is a NULL pointer called
"varnish_instance_name". Would it be reasonable to let the user
configure something else here and use NULL as a default?
* When "VSL_OpenStats" fails, is it possible to get a more detailed
error description or at least an error code?
* The member (value_list_t *)->values_len is of type "int". You assign
to it a "size_t" without cast, which may result in warnings (and hence
problems) with some optimization settings.
Best regards,
—octo
[*] In the long run, I'd prefer to have *one* type for this purpose that
is used by all plugins. I might change that in an attempt to unify
the behavior eventually.
--
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
signature.asc
Description: Digital signature
_______________________________________________ collectd mailing list collectd@verplant.org http://mailman.verplant.org/listinfo/collectd
