[rrd-developers] [PATCH 2/5] src/rrd_daemon.c: Add the "FETCH" command.

[Florian Forster]

Hi Kevin,

On Sat, May 23, 2009 at 04:57:03PM -0500, kevin brintnall wrote:
> On Sat, May 23, 2009 at 01:04:25PM +0200, Florian Forster wrote:
> > +static int handle_request_fetch (listen_socket_t *sock, /* {{{ */
> > +    char *buffer, size_t buffer_size)
> 
> For all handler functions, we should use HANDLER_PROTO in the
> declaration.  See the other handlers.

oh, yeah, that was a quick rebase.. I remembered adding the command to
the appropriate array but forgot to adapt to that patch. I'll send
updated patches in a short while.

> >   * Authors:
> 
> Please leave as-is.

Okay.

> > +static int handle_request_fetch (listen_socket_t *sock, /* {{{ */
> > +    char *buffer, size_t buffer_size)
> > +{
> >
> > ...
> >
> > +  status = flush_file (file);
> 
> Maybe we should check the start/end times, compared to the
> cache_item->last_flush_time.  When calling "FETCH" on really old
> values, flush would be unnecessary.  Can we do it without knowing each
> file's rrd step value?

I think this is not easy implement and has an advantage in only rare
cases. I'd leave such optimizations until someone really does need them.
Until then, they only make the code hard to read.

> > +#define SSTRCAT(buffer,str,buffer_fill) do { \
> > +    size_t str_len = strlen (str); \
> > +    if ((buffer_fill + str_len) > sizeof (buffer)) \
> > +      str_len = sizeof (buffer) - buffer_fill; \
> > + ...
> 
> Why not strlcat?

Because afaIk `strlcat' is a BSD extension and not in ISO C, POSIX or an
X/Open extension. Some system is bound to not have this extension (the
GNU libc, for example) and then we need to provide a work-around anyway,
so imho it's easier to stick to standards in the first place.

An alternative would be to check for strlcat in the configure script and
roll our own version if it is not available.

> > +      snprintf (tmp, sizeof (tmp), " %0.10e", *data_ptr);
> > +      tmp[sizeof (tmp) - 1] = 0;
> 
> snprintf always NULL terminates, the tmp[]=0 is superfluous.

That's right, but at least the GNU libc manpage isn't entirely clear on
that (the POSIX manpage doesn't leave a doubt, though). So I rather
don't take any chances. Especially since the byte I write to here is
already in the CPU cache - the performance implications are negligible
and it is easy for anyone to see that the buffer will be null-terminated
in any case.

> Docs patch for the "FLUSH" command?

I'll send a patch soon.

Thanks for your feedback :)

Regards,
-octo
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.oetiker.ch/pipermail/rrd-developers/attachments/20090525/ce4507b6/attachment.bin