[collectd] [PATCH] Plugin for monitoring TokyoTyrant

[Sebastian Harl]

Hi Paul,

On Thu, Jun 18, 2009 at 02:58:29PM -0600, Paul Sadauskas wrote:
> I'm very rusty at my C, and I really don't understand how the build system
> here works.

Don't worry, we can take care of that.

A small advice regarding the configuration: The tokyotyrant upstream
source package includes a tokyotyrant.pc file which can be used by
pkg-config to determine the required libraries automatically. In
configure.in, pkg-config can be called using the PKG_CHECK_MODULES()
marco. Have a look at the pkg-config(1) manpage for details or see
configure.in for some examples.

See a few, rather minor comments below.

> On Thu, Jun 18, 2009 at 2:55 PM, Paul Sadauskas <psadauskas at gmail.com>wrote:
> > diff --git a/configure.in b/configure.in
> > index 51e3803..a663449 100644
> > --- a/configure.in
> > +++ b/configure.in
> > @@ -3891,6 +3920,7 @@ Configuration:
> >     librrd  . . . . . . . $with_librrd
> >     libsensors  . . . . . $with_libsensors
> >     libstatgrab . . . . . $with_libstatgrab
> > +    libtokyotyrant  . . . $with_libtokyotyrant
> >     libupsclient  . . . . $with_libupsclient
> >     libvirt . . . . . . . $with_libvirt
> >     libxml2 . . . . . . . $with_libxml2

Nitpick: Please include the plugin in the configuration summary as well
;-)

> > diff --git a/src/tokyotyrant.c b/src/tokyotyrant.c
> > new file mode 100644
> > index 0000000..3ac0f97
> > --- /dev/null
> > +++ b/src/tokyotyrant.c
> > +static int tt_config (const char *key, const char *value);
> > +static int tt_read (void);
> > +static void tt_submit(gauge_t rnum, const char *type);

Usually (within collectd), functions are declared in the "right" order
and, thus, forward declarations are not needed. That's mostly a matter
of personal preference though, so don't worry about it.

> > +static int tt_config (const char *key, const char *value)
> > +{
> > +
> > +  if (strcasecmp ("Host", key) == 0)
> > +  {
> > +    if (host != NULL)
> > +      free (host);
> > +    host = strdup(value);
> > +  }

By including an else-case which returns -1, you'll be able to detect
errors more easily.

> > +static int tt_read (void) {
[...]
> > +  rnum = tcrdbrnum(rdb);
> > +  size = tcrdbsize(rdb);
> > +
> > +  if (!tcrdbclose(rdb))
> > +  {
> > +    printerr (rdb);
> > +    tcrdbdel (rdb);
> > +    return (1);
> > +  }
> > +  tt_submit (rnum, "records");
> > +  tt_submit (size, "file_size");

Is there any reason for doing it this way around (i.e. submitting the
values only if close() succeeded)? I cannot image that an error during
close() would affect the values that have been queried before. Checking
for errors in tcrdbrnum() and tcrdbsize() seems more appropriate to me.

> > +static void tt_submit (gauge_t val, const char* type)
[...]
> > +  sstrncpy (vl.host, hostname_g, sizeof (vl.host));
[...]
> > +  sstrncpy (vl.plugin_instance, host, sizeof (vl.plugin_instance));

Imho, 'host' (without the port) should be stored in vl.host (and
vl.plugin_instance should be the empty string).

Cheers,
Sebastian

-- 
Sebastian "tokkee" Harl +++ GnuPG-ID: 0x8501C7FC +++ http://tokkee.org/

Those who would give up Essential Liberty to purchase a little Temporary
Safety, deserve neither Liberty nor Safety.         -- Benjamin Franklin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20090619/b2767db6/attachment.pgp