[collectd] [PATCH] Make tcpconns run on OpenBSD

[Sebastian Harl]

Hi Michael,

On Sat, Aug 09, 2008 at 02:44:37PM +0200, Michael Stapelberg wrote:
> > Which parts of your patch does the licence apply to?
> Basically, I ripped this code out of netstat (and cleaned it quite a
> bit/stripped it to what we need). So, the BSD license and the copyright I
> pasted is for everything in this patch.

Okay, so the copyright information should be added to src/tcpconns.c - I
will take care of that. The file will then be distributed in parts under
the GPL and in the other parts under the BSD license but that's fine ;-)

> > 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
> I wrote the plugin as a standalone proof-of-concept before so I needed all of
> them then ;-). You are right, I could remove some of them.

There are still some more that can be removed and some of the others
should probably go to collectd.h. Anyway, I'll take care of that as
well.

> > > +kvm_t *kvmd;
> > This should be made static.
> Yes, I forgot that, sorry.
> 
> > > +struct nlist nl[] = {

... and here's another one ;-)

> > > +#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.
> Well, this was a strange issue. I first tried to use the struct without the
> defines, but then it just did not work (getting the memory areas for the
> kvm-calls into this list). Maybe there is some evil magic going on in the
> OpenBSD's nlist.h internals, I'm not sure. I'd rather keep them.

Uh, that sounds really strange. I will setup an OpenBSD box within the
next few days and take a look at that myself.

> > 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?
> Sure. The array will be filled with memory offsets of where the data is which
> you want to get. In this case it will be filled with the offset of the list of
> TCP connections which is then iterated.

Hrm... "interesting" concept ;-) Thanks for the explanation!

> > 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?
> Order definitely matters. As written above, even the defines matter somehow.
> I'm not sure about the version of kvm and couldn't find anything about it in
> the original netstat code. I think this should be backwards compatible.

Sounds quite error prone to me, so I surely hope that we won't have to
adopt this a lot in the future. Did you have a look at the kvm
documentation regarding backward compatibility?

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/20080810/b4db8c34/attachment.pgp