Hi José, On Sun, Sep 07, 2008 at 09:03:41PM +0200, José Luis Tallón wrote: > >> The strategy we used was to re-design everything in C++, > >> object-oriented, and then code the resulting "projection" onto C.
I like your general approach, especially the object oriented approach
which fits nicely for librrd.
Of course, as you predicted, not everybody likes everything about it, so
here I go: ;)
> The handle types are typedef'd (void*) pointers,
I much prefer to use `real' opaque handles. Something like:
-- 8< --
struct rrd_s;
typedef struct rrd_s rrd_t;
rrd_t *rrd_open (const char *file);
-- >8 --
I prefer that, because void-pointers can be assigned to anything, so
whatever_t *foo = (rrdng_rrd_t) bar;
doesn't result in an error. Also, if you define `struct rrd_s' in an
internal header, no casting is required internally, because the pointer
is the right type to begin with.
Last but not least, I think typedefs to pointers are confusing and make
the code hard(er) to read..
> The variadic interfaces (stdarg-based) are provided for programmer's
> convenience: when we already have all parameters, it is easier to just
> make one call with all required data and have the RRD created than
> making, say, 10 calls to fill in all attributes.
This kind of sabotages the advantage you gain by replacing the argv-
interface. To set many aspects at once, I'd rather use a pointer to a
struct, like the `hints' passed to `getaddrinfo(3)'. For example:
-- 8< --
#define RRD_SETTINGS_INIT { 300, time (NULL) - 10, ... }
rrd_settings_t settings = RRD_SETTINGS_INIT;
settings.step = 30;
/* settings.start: Default set by initialization macro. */
...
status = rrd_create (filename, &settings,
(sizeof (ds) / sizeof (*ds)), ds,
(sizeof (rra) / sizeof (*rra)), ds);
-- >8 --
Right now, this interface isn't really necessary yet: There are two
options to `rrd_create' right now, that's easy enough. But if new
options are added this approach is *much* easier to extend without
breaking existing code. In the example above, imagine a new option /
member is added to `rrd_settings_t' - if the code isn't changed, the
default will be used. Without touching one line of code!
The rest of the interface should use the same mechanism: Put all the
*optional* arguments in a struct, but pass *required* arguments
directly.
My last point, and here I really dive into personal preference without
any good arguments for it: Mixing names such as `librrd_version' with
``smallCapsNames'' is horrible. I don't like the caps approach much, so
I'd much prefer `rrd_get_ds_count' over `librrd_countDSs'. Actually, I'd
simply change that function to:
int rrd_get_ds (rrd_t *, size_t *ds_count_ret, rrd_ds_t * const *ds_ret);
(Hope I didn't get the `const' wrong..)
I've done a quick mockup, how I'd design the API. I didn't even try to
compile anything, so there may be syntax errors and stuff all around.
It's just to get the basic idea(s) across. Any ideas / comments?
Regards,
-octo
--
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
/**
* RRDTool - API for the rrd library.
* Copyright (C) 2008 Florian Forster <octo at verplant.org>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#ifndef RRD_H
#define RRD_H 1
/* API version. 1004000 == 1.4.0 */
#define RRD_VERSION 1004000
/*
* Globally used types.
*/
/* Opaque RRD handle */
struct rrd_s;
typedef struct rrd_s rrd_t;
/* Type of values used internally */
typedef double rrd_value_t;
#define RRD_DS_NAME_LENGTH 128
#define RRD_DS_TYPE_GAUGE 0
#define RRD_DS_TYPE_COUNTER 1
#define RRD_DS_TYPE_DERIVE 2
#define RRD_DS_TYPE_ABSOLUTE 3
#define RRD_DS_TYPE_COMPUTE 4
/* Data specific to each type of data source */
struct rrd_ds_data_standard_s
{
int heartbeat;
rrd_value_t min;
rrd_value_t max;
};
typedef struct rrd_ds_data_standard_s rrd_ds_data_standard_t;
struct rrd_ds_data_compute_s
{
rrd_cdef_t *cdef;
};
typedef struct rrd_ds_data_compute_s rrd_ds_data_compute_t;
union rrd_ds_data_u
{
rrd_ds_data_standard_t standard;
rrd_ds_data_compute_t compute;
};
typedef union rrd_ds_data_u rrd_ds_data_t;
/* Type for data sources */
struct rrd_ds_s
{
char name[RRD_DS_NAME_LENGTH];
int type;
rrd_ds_data_t data;
};
typedef struct rrd_ds_s rrd_ds_t;
/* TODO: Likewise for RRAs */
/*
* Constructor / Deconstructor
*/
/* File creation */
#define RRD_CREATE_OPTS_INIT { 300, 0 }
struct rrd_create_opts_s
{
unsigned int step;
time_t start; /* 0 => `now - 10 seconds' */
};
typedef struct rrd_create_opts_s rrd_create_opts_t;
rrd_t *rrd_create (const char *filename, const rrd_create_opts_t *opts,
size_t ds_num, const rrd_ds_t * const *ds,
size_t rra_num, const rrd_rra_t * const *rra);
/* Open an existing file */
rrd_t *rrd_open (const char *filename);
int rrd_close (rrd_t *rrd);
#define RRD_DESTROY(h) do { rrd_close (h); (h) = NULL; } while (0)
/*
* Functions / Methods
*/
/* Update values */
#define RRD_UPDATE_OPTS_INIT { 0, NULL }
struct rrd_update_opts_s
{
time_t *timestamps;
char **template_ds;
};
typedef struct rrd_update_opts_s rrd_update_opts_t;
/* Values is a pointer to `updates_num' `rrd_value_t *' pointers, each
* pointing to `values_per_update' values. */
int rrd_update (rrd_t *, const rrd_update_opts_t *opts,
size_t updates_num, size_t values_per_update,
const rrd_value_t * const *values);
/* TODO: All the other functions not covered by this example */
#endif /* RRD_H */
signature.asc
Description: Digital signature
_______________________________________________ rrd-developers mailing list [email protected] https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
