[collectd] Collectd bind plugin patch: timesaving problem

[Bruno Prémont]

Hi,

On Fri, 24 Jun 2011 13:02:35 Sebastian Harl wrote:
> On Fri, Jun 24, 2011 at 12:28:22PM +0200, Bruno Prémont wrote:
> > On Fri, 24 Jun 2011 12:10:41 Aurelien ROUGEMONT wrote:
> > > I'm working for a registrar. We use intensively isc-bind, and since a
> > > few month collectd's bind plugin.
> > > 
> > > We were experiencing a very specific behavior from this plugin only. The
> > > datetime returned was shifted. After some diggin', i figured what was
> > > the problem : the current code was not handling timesavings.
> > > 
> > > the bind plugin works like this :
> > >  * query to xmlrpc bind webservice
> > >  * process data : bind statistics and datetime
> > >  * returns processed data
> > > 
> > > Since it's the only plugin returning a datetime not using timesavings i
> > > read [1] and wrote a quick fix that :
> > > 
> > > 1- substracts the constant timezone to the "about to be returned datetime"
> > > 2- adds to the result timezone and timesaving ( with *localtime() )
> > > 3- returns the result instead of the UTC datetime + timezone

The datetime returned by Bind is UTC but conversion to time_t via
localtime() offsets by timezone AND daylight saving.

> > > [1]  http://www.gnu.org/s/hello/manual/libc/Time-Zone-Functions.html
> > 
> > The more simple/safe solution should be to revert timegm() -> mktime()
> > change done in
> > http://git.verplant.org/?p=collectd.git;a=commitdiff;h=1641c82ec4e14968ea31021dfb8b520df5f4483a
> 
> timegm() is a GNU extension, thus, using it should be avoided. So, imho,
> the approach taken in the patch is fine.

timegm() is also available on some of the BSDs.
I would say it's better to use timegm() when available and fallback to
some workaround when not available instead of just doing work-around.

> > In your patch you refer to a "timezone" variable, where does that one
> > come from?
> 
> It's defined in 'time.h' -- it's defined as a mandatory XSI extension of
> POSIX.

Ok, it has a man-page too but there it's mentioned as:
  timezone: _SVID_SOURCE || _XOPEN_SOURCE
which is not POSIX, just XOPEN_SOURCE (and _SVID_SOURCE also provides
timegm())

Then timezone is no enough, daylight-saving must be accounted for
as well (and that starts to get painfull).

> > In addition, localtime() and friends depend on process-global timezone
> > setting which is rather bad in a multithreaded process such as collectd!
> 
> Hrm … do you really consider that to be an issue? Imho, having the
> timezone a process-global setting is fine even in a multi-threaded
> process. I'd rather consider code buggy that touches that setting ;-)

As long as timezone is never changed it's fine (but who knows what some
of the libraries used by plugins might do).

Bruno