Hi Keith, thank you very much for your work!
On Wed, Aug 03, 2011 at 01:23:14PM -0700, Keith Chambers wrote: > I understand that this will not get merged in to the mainline due to > the associated VMware licensing. I don't see any licensing problems regarding including this plugin in the main distribution. Since we're not distributing binary packages, even an incompatible license wouldn't necessarily keep us from including that file in the distribution. That being said, having compatible licenses is obviously desirable ;) > diff -Nur a/configure b/configure This is an automatically generated file. > diff -Nur a/configure.in b/configure.in > --- a/configure.in 1970-01-01 00:00:00.000000000 +0000 > +++ b/configure.in 1970-01-01 00:00:00.000000000 +0000 > @@ -4350,6 +4352,7 @@ > AC_PLUGIN([users], [$plugin_users], [User statistics]) > AC_PLUGIN([uuid], [yes], [UUID as hostname plugin]) > AC_PLUGIN([vmem], [$plugin_vmem], [Virtual memory statistics]) > +AC_PLUGIN([vmware], [$plugin_vmware], [VMware client statistics]) > AC_PLUGIN([vserver], [$plugin_vserver], [Linux VServer statistics]) > AC_PLUGIN([wireless], [$plugin_wireless], [Wireless statistics]) > AC_PLUGIN([write_http], [$with_libcurl], [HTTP output plugin]) You're not checking for the availability of the library to the linker, but loading all required symbols yourself using dlsym(3) and friends. I'd prefer if we could actually link against that library and let ld.so do all the work. I don't have any experience with the VMWare API, but from a quick glance it looks like this would be possible. What do you think? If there is a check for the library, instead of $plugin_vmware you should pass $have_libvmware (or something along these lines) to automatically enable the plugin *if* the library has been found. > diff -Nur a/src/config.h.in b/src/config.h.in This file is automatically generated. > diff -Nur a/src/types.db b/src/types.db > --- a/src/types.db 1970-01-01 00:00:00.000000000 +0000 > +++ b/src/types.db 1970-01-01 00:00:00.000000000 +0000 > @@ -150,6 +151,7 @@ > threads value:GAUGE:0:U > time_dispersion value:GAUGE:-1000000:1000000 > timeleft value:GAUGE:0:3600 > +time value:DERIVE:0:U > time_offset value:GAUGE:-1000000:1000000 > total_bytes value:DERIVE:0:U > total_connections value:DERIVE:0:U Please use the already existing type "total_time_in_ms" instead. > @@ -168,6 +170,8 @@ > vmpage_faults minflt:DERIVE:0:U, majflt:DERIVE:0:U > vmpage_io in:DERIVE:0:U, out:DERIVE:0:U > vmpage_number value:GAUGE:0:4294967295 > +vmware_qos value:GAUGE:0:U > +vmware_host_cpu value:GAUGE:0:U > volatile_changes value:GAUGE:0:U > voltage_threshold value:GAUGE:U:U, threshold:GAUGE:U:U > voltage value:GAUGE:U:U I think it'd make sense to reuse the existing type "vcpu" rather than introducing a VMWare-specific type. What do you think? > diff -Nur a/src/vmware.c b/src/vmware.c > --- a/src/vmware.c 2011-05-22 02:15:32.179086658 +0000 > +++ b/src/vmware.c 2011-05-22 02:12:35.340762567 +0000 This file is missing a license and copyright header. > @@ -0,0 +1,395 @@ > +#include <stdlib.h> > +#include <stdarg.h> > +#include <stdio.h> > +#include <dlfcn.h> > +#include "collectd.h" > +#include "common.h" > +#include "plugin.h" "collectd.h" *must* be the first file to be included. It may set defines which configure the standard library. > +/* handle for use with shared library */ > +void *dlHandle = NULL; This vaiable is not used outside of "LoadFunctions". Please make it an automatically scoped (stack) variable. > +VMGuestLibError glError; This variable doesn't need to be global. Please make is an automatically scoped (stack) variable. > + return FALSE; \ Please don't use the TRUE and FALSE macros. Use 1 and 0 instead. > +Bool > +LoadFunctions(void) Please use the C99 built-in type "_Bool" instead of "Bool". Also, this function should be static. > +static int vmware_init (void) > +{ > + if (!LoadFunctions()) { > + ERROR ("vmware guest plugin: Unable to load GuistLib functions"); s/GuistLib/GuestLib/ > + return (-1); > + } > + > + /* try to load the library */ > + glError = GuestLib_OpenHandle(&glHandle); > + if (glError != VMGUESTLIB_ERROR_SUCCESS) { > + ERROR ("OpenHandle failed: %s", GuestLib_GetErrorText(glError)); Please use a common prefix for all messages, for example "vmware guest plugin:". > + if (sessionId == 0) { > + sessionId = tmpSession; > + DEBUG ("Initial session ID is 0x%"FMT64"x", sessionId); Since the API makes no guarantee about the type of VMSessionId, I'd prefer to case here. Also, FMT64 is some VMWare trickery, I'd rather use the C99 standard: DEBUG ("Initial session ID is %#"PRIx64, (uint64_t) sessionId); > + submit_vmw_counter ("cpu", "used_ms", value); > + submit_vmw_counter ("cpu", "stolen_ms", value); I'd drop the "…_ms" in the type instance. > + submit_vmw_gauge ("vmware_qos", "reservation_mhz", value); > + submit_vmw_gauge ("vmware_qos", "limit_mhz", value); > + if (glError != VMGUESTLIB_ERROR_SUCCESS) { > + DEBUG ("Failed to get cpu shares: %s\n", > GuestLib_GetErrorText(glError)); > + } > + value = (gauge_t) cpuShares; > + submit_vmw_gauge ("vmware_qos", "shares", value); The first two "vmware_qos" values appear to be in MHz, the last seems to be something else (fraction of CPUs?). They shouldn't have the same type. "value" is of type "derive_t", but the third argument to "submit_vmw_gauge" is of type "gauge_t". Please don't make this an implicit cast. I'd use "derive_t dval" and "gauge_t gval" instead or just add the cast to the submit_vmw_{derive,gauge}() invocation, for example: submit_vmw_gauge ("vmware_qos", "reservation_mhz", (gauge_t) cpuReservationMHz); > + submit_vmw_gauge ("vmware_host_cpu", "clock_speed_mhz", value); Please use the existing type "cpufreq" instead. It records the CPU frequency in Hertz, so the call should be something like: submit_vmw_gauge ("cpufreq", "", 1.0e6 * value); > + submit_vmw_gauge ("memory", "target_mb", value); > + submit_vmw_gauge ("memory", "used_mb", value); > + submit_vmw_gauge ("memory", "mapped_mb", value); > + submit_vmw_gauge ("memory", "active_mb", value); > + submit_vmw_gauge ("memory", "overhead_mb", value); > + submit_vmw_gauge ("memory", "shared_mb", value); > + submit_vmw_gauge ("memory", "shared_saved_mb", value); > + submit_vmw_gauge ("memory", "ballooned_mb", value); > + submit_vmw_gauge ("memory", "swapped_mb", value); Please convert to bytes, i.e. multiply by 2^20. The graphing front-ends should take care to convert that back to MByte / GByte. Also, lose the "…_mb" suffix. > + submit_vmw_gauge ("vmware_qos", "reservation_mb", value); > + submit_vmw_gauge ("vmware_qos", "limit_mb", value); This time "vmware_qos" is MBytes. Please remove the "…_mb" suffix and use a type that describes the type of data. You could use "memory" again and set a different plugin instance, for example. > + submit_vmw_gauge ("vmware_qos", "shares", value); A value with this name has already been submitted. What kind of data is is this time? Fractions of memory? Number of pages? Best regards and thanks again for all the work you've put into this! —octo -- Florian octo Forster Hacker in training GnuPG: 0x0C705A15 http://octo.it/
signature.asc
Description: Digital signature
_______________________________________________ collectd mailing list collectd@verplant.org http://mailman.verplant.org/listinfo/collectd