[collectd] [PATCH] Make tcpconns run on OpenBSD

[Sebastian Harl]

Hi Michael,

Thanks for your patch!

On Fri, Aug 08, 2008 at 03:50:25AM +0200, Michael Stapelberg wrote:
> I've just integrated OpenBSD-support into the tcpconns module. Please test
> this thouroughly, it's not running for so long on my box (but works fine).

Hrm... I'm not currently using collectd on OpenBSD and I don't think
Florian is doing so. Anybody else out there who wants to give that patch
a try and provide some feedback?

> Also, please add -lkvm to the right place in the autoconf-stuff so that when
> using tcpconns on OpenBSD, libkvm is linked.

Will do so after this patch has been applied.

> Also, I'm not sure about how much of the license we need to embed. I'll
> therefore post the full license information so you can put it in if you feel
> like:

Which parts of your patch does the licence apply to?

I've got a few quick comments about your patch - please see below.

> diff --git a/tcpconns.c b/tcpconns.c
> index 655c53e..ea0ad79 100644
> --- a/tcpconns.c
> +++ b/tcpconns.c

> +#elif __OpenBSD__
> +
> +# include <sys/queue.h>
> +# include <sys/types.h>
> +# include <sys/socket.h>
> +# include <net/route.h>
> +# include <netinet/in.h>
> +# include <netinet/in_systm.h>
> +# include <netinet/ip.h>
> +# include <netinet/in_pcb.h>
> +# include <netinet/tcp.h>
> +# include <netinet/tcp_timer.h>
> +# include <netinet/tcp_var.h>
> +# include <netdb.h>
> +# include <arpa/inet.h>
> +# include <nlist.h>
> +# include <kvm.h>
> +# include <fcntl.h>
> +# include <limits.h>
> +#endif /* __OpenBSD__ */

Uh... I can hardly image that you need all of those headers. Also, most
of them are already included by collectd.h. The ones that are missing
there should be added to that header as well instead of the plugin.
Could you please change that? We will then gladly add appropriate checks
to configure.

> +kvm_t *kvmd;

This should be made static.

> +struct nlist nl[] = {
> +#define N_MBSTAT        0
> +        { "_mbstat" },
[...]

I don't see you using any of those defines. Is there any reason to keep
them or could they be removed? Again, the array should be declared
static.

Frankly, I'm too lazy to look into the kvm documentation right now.
Could you please explain in a few words what this array is used for?
Right now, this looks like a whole lot of magic to me ;-) Also, it would
be interesting to know how that array depends on the version of kvm,
i.e. how likely do we run into problems if people use different versions
of kvm. Does order matter?

Besides that, the patch looks fine to me from a quick glance and without
looking into the kvm stuff. If you fix those issues the patch would be
fine for inclusion from my point of view.

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: 189 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20080809/f1484205/attachment.pgp