[collectd] [PATCH] Interface option for network plugin

[Max Henkel]

Hi octo and list!

On Tue, Mar 09, 2010 at 06:17:35PM +0100, Florian Forster wrote:
[...]
> On Fri, Feb 26, 2010 at 12:49:02PM +0100, Max Henkel wrote:
> > +#if KERNEL_LINUX
> > +		struct ip_mreqn mreq;
> > +#else
> > +		struct ip_mreq mreq;
> > +#endif
> 
> As far as I see, the only difference between the two structs is the
> "imr_address", which you later set to INADDR_ANY. Couldn't we remove the
> #if altogether and use "struct ip_mreq" on all platforms?

"struct ip_mreq" is indeed supported on all platforms, but it will
only allow you to set a source IP address (and specifying by it the
intended interface). The idea was to use the interface index, since
it is also compatible with the IPv6 style of setting it. Furthermore
it avoids the use of an ioctl (SIOCGIFCONF) that might not be very
portable.

Anyway the "struct ip_mreqn" is supported at least since Linux
Kernel 2.4 and FreeBSD 7.0 (for outgoing multicast traffic).

> > +		if (! IN_MULTICAST (ntohl (addr->sin_addr.s_addr)))
> > +			return (0);
> 
> Doesn't it make sense to be able to set the interface in unicast mode,
> too? For example if the host has multiple default gateways. The
> socket(7) option "SO_BINDTODEVICE" could be used for this I guess.

You're right concerning the multi-gateway scenario. I haven't
touched it, just because the problem is getting very obvious for
multicast traffic (but haven't considered this case). Thus I will
try to enhance it whith this functionality.

> > @@ -2842,6 +2929,8 @@ static int network_config (oconfig_item_t *ci) /* {{{ */
> >        network_config_add_server (child);
> >      else if (strcasecmp ("TimeToLive", child->key) == 0)
> >        network_config_set_ttl (child);
> > +    else if (strcasecmp ("Interface", child->key) == 0)
> > +      network_config_set_interface (child);
> >      else if (strcasecmp ("MaxPacketSize", child->key) == 0)
> >        network_config_set_buffer_size (child);
> >      else if (strcasecmp ("Forward", child->key) == 0)
> 
> Starting with the signing and encryption option, we have <Server> and
> <Listen> blocks that take socket specific options. It'd be kind of
> inconsistent with the "TimeToLive" and other options, but maybe adding
> this to those blocks would be even better. It would enable to forward
> data from one multicast group on interface-0 to the *same* multicast
> group on interface-1, which I guess could be useful. But maybe it's an
> academic case, I dunno.. Thoughts, anyone?

I've already thought about that, but I've expected that this style
is expected in the config file (Apache style ;-) and thus the patch
resembles this behaviour. If this is not the case I'm going to
correct this and move the global variable whith to the sockent_t as
well.

I hope I've clarified some of my ideas for this approach. :-)

Best regards,

Max
-- 
< henkel at gmx dot at >