Suggestion for API extension

62 messages Options
Embed this post
Permalink
1 2 3 4
Benny Baumann

Suggestion for API extension

Reply Threaded More More options
Print post
Permalink
Hi,

I recently discovered the PHP extension for RRDTool which is available
as a contribution to RRDTool. This extension runs successfully with
latest PHP5, but misses some functions of current RRDTool releases (no
rrd_dump, no rrd_info). I'm currently working on updating the PHP
extension to also include these operations.

And here comes the issue: The RRDTool library is more or less written to
be used in the command line interface thus either returns only single
values or dumps output to a file\terminal. For the PHP extension I need
output to be written into a buffer though.

To solve this I suggest adding a callback interface to e.g. rrd_dump
that is called whenever string data (or binary data) has to be returned.
An example implementation for rrd_dump_opt_cb_r can be found in the
attached patch. Feedback appreciated. IDK if this patch is complete and
if the mentioned rrd_dump_opt_cb_r function is properly exported; but at
least dumping should work.

The basic rrd_dump_opt_r now calls this Callback method internally to
give an example of how this callback interface can be used and to avoid
duplicate source.

Regards,
BenBE.

[rrd_dump_cb.patch]


Index: Z:/data/programming/own/rrdtool/trunk/lib/src/rrd_dump.c
===================================================================
--- Z:/data/programming/own/rrdtool/trunk/lib/src/rrd_dump.c (Revision 19)
+++ Z:/data/programming/own/rrdtool/trunk/lib/src/rrd_dump.c (Revision 23)
@@ -58,107 +58,149 @@
 extern char *tzname[2];
 #endif

+//Local prototypes
+size_t rrd_dump_opt_cb_fileout(
+    const void *data,
+    size_t len,
+    void *user);

-int rrd_dump_opt_r(
+
+int rrd_dump_opt_cb_r(
     const char *filename,
-    char *outname,
-    int opt_noheader)
+    int opt_noheader,
+    rrd_output_callback_t cb,
+    void *user)
 {
     unsigned int i, ii, ix, iii = 0;
     time_t    now;
     char      somestring[255];
+    char      somestring_buf[255];
     rrd_value_t my_cdp;
     off_t     rra_base, rra_start, rra_next;
     rrd_file_t *rrd_file;
-    FILE     *out_file;
     rrd_t     rrd;
     rrd_value_t value;
     struct tm tm;
     char *old_locale = "";
+
+    //Check if we got a (valid) callback method
+    if (!cb) {
+        return (-1);
+    }
+
     rrd_file = rrd_open(filename, &rrd, RRD_READONLY | RRD_READAHEAD);
     if (rrd_file == NULL) {
         rrd_free(&rrd);
         return (-1);
     }

-    out_file = NULL;
-    if (outname) {
-        if (!(out_file = fopen(outname, "w"))) {
-            return (-1);
-        }
-    } else {
-        out_file = stdout;
-    }
 #ifdef HAVE_SETLOCALE
     old_locale = setlocale(LC_NUMERIC, "C");
 #endif
+
     if (!opt_noheader) {
-        fputs("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n", out_file);
-        fputs
-            ("<!DOCTYPE rrd SYSTEM \"http://oss.oetiker.ch/rrdtool/rrdtool.dtd\">\n",
-             out_file);
+        cb("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n",
+            strlen("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"), user);
+        cb("<!DOCTYPE rrd SYSTEM \"http://oss.oetiker.ch/rrdtool/rrdtool.dtd\">\n",
+            strlen("<!DOCTYPE rrd SYSTEM \"http://oss.oetiker.ch/rrdtool/rrdtool.dtd\">\n"), user);
     }
-    fputs("<!-- Round Robin Database Dump -->", out_file);
-    fputs("<rrd>", out_file);
+
+    cb("<!-- Round Robin Database Dump -->",
+        strlen("<!-- Round Robin Database Dump -->"), user);
+
+    cb("<rrd>",
+        strlen("<rrd>"), user);
+
     if (atoi(rrd.stat_head->version) <= 3) {
-        fprintf(out_file, "\t<version> %s </version>\n", RRD_VERSION3);
+        snprintf(somestring, 255, "\t<version> %s </version>\n", RRD_VERSION3);
     } else {
-        fprintf(out_file, "\t<version> %s </version>\n", RRD_VERSION);
+        snprintf(somestring, 255, "\t<version> %s </version>\n", RRD_VERSION);
     }
-    fprintf(out_file, "\t<step> %lu </step> <!-- Seconds -->\n",
-            rrd.stat_head->pdp_step);
+    cb(somestring, strlen(somestring), user);
+
+    snprintf(somestring, 255, "\t<step> %lu </step> <!-- Seconds -->\n",
+        rrd.stat_head->pdp_step);
+    cb(somestring, strlen(somestring), user);
+
 #ifdef HAVE_STRFTIME
     localtime_r(&rrd.live_head->last_up, &tm);
-    strftime(somestring, 200, "%Y-%m-%d %H:%M:%S %Z", &tm);
+    strftime(somestring, 255, "%Y-%m-%d %H:%M:%S %Z", &tm);
 #else
 # error "Need strftime"
 #endif
-    fprintf(out_file, "\t<lastupdate> %lu </lastupdate> <!-- %s -->\n\n",
-            (unsigned long) rrd.live_head->last_up, somestring);
+    snprintf(somestring_buf, 255, "\t<lastupdate> %lu </lastupdate> <!-- %s -->\n\n",
+        (unsigned long) rrd.live_head->last_up, somestring);
+    cb(somestring_buf, strlen(somestring_buf), user);
+
     for (i = 0; i < rrd.stat_head->ds_cnt; i++) {
-        fprintf(out_file, "\t<ds>\n");
-        fprintf(out_file, "\t\t<name> %s </name>\n", rrd.ds_def[i].ds_nam);
-        fprintf(out_file, "\t\t<type> %s </type>\n", rrd.ds_def[i].dst);
+        cb("\t<ds>\n",
+            strlen("\t<ds>\n"), user);
+
+        snprintf(somestring_buf, 255, "\t\t<name> %s </name>\n", rrd.ds_def[i].ds_nam);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
+        snprintf(somestring_buf, 255, "\t\t<type> %s </type>\n", rrd.ds_def[i].dst);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
         if (dst_conv(rrd.ds_def[i].dst) != DST_CDEF) {
-            fprintf(out_file,
-                    "\t\t<minimal_heartbeat> %lu </minimal_heartbeat>\n",
+            snprintf(somestring_buf, 255, "\t\t<minimal_heartbeat> %lu </minimal_heartbeat>\n",
                     rrd.ds_def[i].par[DS_mrhb_cnt].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             if (isnan(rrd.ds_def[i].par[DS_min_val].u_val)) {
-                fprintf(out_file, "\t\t<min> NaN </min>\n");
+                snprintf(somestring_buf, 255, "\t\t<min> NaN </min>\n");
             } else {
-                fprintf(out_file, "\t\t<min> %0.10e </min>\n",
-                        rrd.ds_def[i].par[DS_min_val].u_val);
+                snprintf(somestring_buf, 255, "\t\t<min> %0.10e </min>\n",
+                    rrd.ds_def[i].par[DS_min_val].u_val);
             }
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             if (isnan(rrd.ds_def[i].par[DS_max_val].u_val)) {
-                fprintf(out_file, "\t\t<max> NaN </max>\n");
+                snprintf(somestring_buf, 255, "\t\t<max> NaN </max>\n");
             } else {
-                fprintf(out_file, "\t\t<max> %0.10e </max>\n",
-                        rrd.ds_def[i].par[DS_max_val].u_val);
+                snprintf(somestring_buf, 255, "\t\t<max> %0.10e </max>\n",
+                    rrd.ds_def[i].par[DS_max_val].u_val);
             }
+            cb(somestring_buf, strlen(somestring_buf), user);
         } else {        /* DST_CDEF */
             char     *str = NULL;

             rpn_compact2str((rpn_cdefds_t *) &(rrd.ds_def[i].par[DS_cdef]),
-                            rrd.ds_def, &str);
-            fprintf(out_file, "\t\t<cdef> %s </cdef>\n", str);
+                rrd.ds_def, &str);
+
+            //Splitting into 3 writes to avoid allocating memory
+            //This is better compared to snprintf as str may be of arbitrary size
+            cb("\t\t<cdef> ", strlen("\t\t<cdef> "), user);
+            cb(str, strlen(str), user);
+            cb(" </cdef>\n", strlen(" </cdef>\n"), user);
+
             free(str);
         }
-        fprintf(out_file, "\n\t\t<!-- PDP Status -->\n");
-        fprintf(out_file, "\t\t<last_ds> %s </last_ds>\n",
-                rrd.pdp_prep[i].last_ds);
+
+        cb("\n\t\t<!-- PDP Status -->\n",
+            strlen("\n\t\t<!-- PDP Status -->\n"), user);
+        snprintf(somestring_buf, 255, "\t\t<last_ds> %s </last_ds>\n",
+            rrd.pdp_prep[i].last_ds);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
         if (isnan(rrd.pdp_prep[i].scratch[PDP_val].u_val)) {
-            fprintf(out_file, "\t\t<value> NaN </value>\n");
+            snprintf(somestring_buf, 255, "\t\t<value> NaN </value>\n");
         } else {
-            fprintf(out_file, "\t\t<value> %0.10e </value>\n",
-                    rrd.pdp_prep[i].scratch[PDP_val].u_val);
+            snprintf(somestring_buf, 255, "\t\t<value> %0.10e </value>\n",
+                rrd.pdp_prep[i].scratch[PDP_val].u_val);
         }
-        fprintf(out_file, "\t\t<unknown_sec> %lu </unknown_sec>\n",
-                rrd.pdp_prep[i].scratch[PDP_unkn_sec_cnt].u_cnt);
+        cb(somestring_buf, strlen(somestring_buf), user);

-        fprintf(out_file, "\t</ds>\n\n");
+        snprintf(somestring_buf, 255, "\t\t<unknown_sec> %lu </unknown_sec>\n",
+            rrd.pdp_prep[i].scratch[PDP_unkn_sec_cnt].u_cnt);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
+        cb("\t</ds>\n\n",
+            strlen("\t</ds>\n\n"), user);
     }

-    fputs("<!-- Round Robin Archives -->", out_file);
+    cb("<!-- Round Robin Archives -->",
+        strlen("<!-- Round Robin Archives -->"), user);

     rra_base = rrd_file->header_len;
     rra_next = rra_base;
@@ -170,182 +212,224 @@
         rra_start = rra_next;
         rra_next += (rrd.stat_head->ds_cnt
                      * rrd.rra_def[i].row_cnt * sizeof(rrd_value_t));
-        fprintf(out_file, "\t<rra>\n");
-        fprintf(out_file, "\t\t<cf> %s </cf>\n", rrd.rra_def[i].cf_nam);
-        fprintf(out_file,
-                "\t\t<pdp_per_row> %lu </pdp_per_row> <!-- %lu seconds -->\n\n",
-                rrd.rra_def[i].pdp_cnt,
-                rrd.rra_def[i].pdp_cnt * rrd.stat_head->pdp_step);
+
+        cb("\t<rra>\n",
+            strlen("\t<rra>\n"), user);
+
+        snprintf(somestring_buf, 255,
+            "\t\t<cf> %s </cf>\n", rrd.rra_def[i].cf_nam);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
+        snprintf(somestring_buf, 255,
+            "\t\t<pdp_per_row> %lu </pdp_per_row> <!-- %lu seconds -->\n\n",
+            rrd.rra_def[i].pdp_cnt,
+            rrd.rra_def[i].pdp_cnt * rrd.stat_head->pdp_step);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
         /* support for RRA parameters */
-        fprintf(out_file, "\t\t<params>\n");
+        cb("\t\t<params>\n",
+            strlen("\t\t<params>\n"), user);
+
         switch (cf_conv(rrd.rra_def[i].cf_nam)) {
         case CF_HWPREDICT:
         case CF_MHWPREDICT:
-            fprintf(out_file, "\t\t<hw_alpha> %0.10e </hw_alpha>\n",
+            snprintf(somestring_buf, 255, "\t\t<hw_alpha> %0.10e </hw_alpha>\n",
                     rrd.rra_def[i].par[RRA_hw_alpha].u_val);
-            fprintf(out_file, "\t\t<hw_beta> %0.10e </hw_beta>\n",
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255, "\t\t<hw_beta> %0.10e </hw_beta>\n",
                     rrd.rra_def[i].par[RRA_hw_beta].u_val);
-            fprintf(out_file,
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255,
                     "\t\t<dependent_rra_idx> %lu </dependent_rra_idx>\n",
                     rrd.rra_def[i].par[RRA_dependent_rra_idx].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
             break;
         case CF_SEASONAL:
         case CF_DEVSEASONAL:
-            fprintf(out_file,
+            snprintf(somestring_buf, 255,
                     "\t\t<seasonal_gamma> %0.10e </seasonal_gamma>\n",
                     rrd.rra_def[i].par[RRA_seasonal_gamma].u_val);
-            fprintf(out_file,
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255,
                     "\t\t<seasonal_smooth_idx> %lu </seasonal_smooth_idx>\n",
                     rrd.rra_def[i].par[RRA_seasonal_smooth_idx].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             if (atoi(rrd.stat_head->version) >= 4) {
-                fprintf(out_file,
-                        "\t\t<smoothing_window> %0.10e </smoothing_window>\n",
-                        rrd.rra_def[i].par[RRA_seasonal_smoothing_window].
-                        u_val);
+                snprintf(somestring_buf, 255,
+                    "\t\t<smoothing_window> %0.10e </smoothing_window>\n",
+                    rrd.rra_def[i].par[RRA_seasonal_smoothing_window].u_val);
+                cb(somestring_buf, strlen(somestring_buf), user);
             }
-            fprintf(out_file,
+            snprintf(somestring_buf, 255,
                     "\t\t<dependent_rra_idx> %lu </dependent_rra_idx>\n",
                     rrd.rra_def[i].par[RRA_dependent_rra_idx].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
             break;
         case CF_FAILURES:
-            fprintf(out_file, "\t\t<delta_pos> %0.10e </delta_pos>\n",
+            snprintf(somestring_buf, 255, "\t\t<delta_pos> %0.10e </delta_pos>\n",
                     rrd.rra_def[i].par[RRA_delta_pos].u_val);
-            fprintf(out_file, "\t\t<delta_neg> %0.10e </delta_neg>\n",
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255, "\t\t<delta_neg> %0.10e </delta_neg>\n",
                     rrd.rra_def[i].par[RRA_delta_neg].u_val);
-            fprintf(out_file, "\t\t<window_len> %lu </window_len>\n",
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255, "\t\t<window_len> %lu </window_len>\n",
                     rrd.rra_def[i].par[RRA_window_len].u_cnt);
-            fprintf(out_file,
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255,
                     "\t\t<failure_threshold> %lu </failure_threshold>\n",
                     rrd.rra_def[i].par[RRA_failure_threshold].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             /* fall thru */
         case CF_DEVPREDICT:
-            fprintf(out_file,
+            snprintf(somestring_buf, 255,
                     "\t\t<dependent_rra_idx> %lu </dependent_rra_idx>\n",
                     rrd.rra_def[i].par[RRA_dependent_rra_idx].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
             break;
         case CF_AVERAGE:
         case CF_MAXIMUM:
         case CF_MINIMUM:
         case CF_LAST:
         default:
-            fprintf(out_file, "\t\t<xff> %0.10e </xff>\n",
+            snprintf(somestring_buf, 255, "\t\t<xff> %0.10e </xff>\n",
                     rrd.rra_def[i].par[RRA_cdp_xff_val].u_val);
+            cb(somestring_buf, strlen(somestring_buf), user);
             break;
         }
-        fprintf(out_file, "\t\t</params>\n");
-        fprintf(out_file, "\t\t<cdp_prep>\n");
+
+        cb("\t\t</params>\n",
+            strlen("\t\t</params>\n"), user);
+        cb("\t\t<cdp_prep>\n",
+            strlen("\t\t<cdp_prep>\n"), user);
+
         for (ii = 0; ii < rrd.stat_head->ds_cnt; ii++) {
             unsigned long ivalue;

-            fprintf(out_file, "\t\t\t<ds>\n");
+            cb("\t\t\t<ds>\n",
+                strlen("\t\t\t<ds>\n"), user);
             /* support for exporting all CDP parameters */
             /* parameters common to all CFs */
             /* primary_val and secondary_val do not need to be saved between updates
              * so strictly speaking they could be omitted.
              * However, they can be useful for diagnostic purposes, so are included here. */
-            value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt
-                                 + ii].scratch[CDP_primary_val].u_val;
+            value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                scratch[CDP_primary_val].u_val;
             if (isnan(value)) {
-                fprintf(out_file,
-                        "\t\t\t<primary_value> NaN </primary_value>\n");
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<primary_value> NaN </primary_value>\n");
             } else {
-                fprintf(out_file,
-                        "\t\t\t<primary_value> %0.10e </primary_value>\n",
-                        value);
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<primary_value> %0.10e </primary_value>\n", value);
             }
-            value =
-                rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                             ii].scratch[CDP_secondary_val].u_val;
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                scratch[CDP_secondary_val].u_val;
             if (isnan(value)) {
-                fprintf(out_file,
-                        "\t\t\t<secondary_value> NaN </secondary_value>\n");
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<secondary_value> NaN </secondary_value>\n");
             } else {
-                fprintf(out_file,
-                        "\t\t\t<secondary_value> %0.10e </secondary_value>\n",
-                        value);
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<secondary_value> %0.10e </secondary_value>\n", value);
             }
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             switch (cf_conv(rrd.rra_def[i].cf_nam)) {
             case CF_HWPREDICT:
             case CF_MHWPREDICT:
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_intercept].u_val;
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_intercept].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file, "\t\t\t<intercept> NaN </intercept>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<intercept> NaN </intercept>\n");
                 } else {
-                    fprintf(out_file,
-                            "\t\t\t<intercept> %0.10e </intercept>\n", value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<intercept> %0.10e </intercept>\n", value);
                 }
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_last_intercept].u_val;
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_last_intercept].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file,
-                            "\t\t\t<last_intercept> NaN </last_intercept>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_intercept> NaN </last_intercept>\n");
                 } else {
-                    fprintf(out_file,
-                            "\t\t\t<last_intercept> %0.10e </last_intercept>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_intercept> %0.10e </last_intercept>\n", value);
                 }
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_slope].u_val;
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_slope].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file, "\t\t\t<slope> NaN </slope>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<slope> NaN </slope>\n");
                 } else {
-                    fprintf(out_file, "\t\t\t<slope> %0.10e </slope>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<slope> %0.10e </slope>\n", value);
                 }
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_last_slope].u_val;
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_last_slope].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file,
-                            "\t\t\t<last_slope> NaN </last_slope>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_slope> NaN </last_slope>\n");
                 } else {
-                    fprintf(out_file,
-                            "\t\t\t<last_slope> %0.10e </last_slope>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_slope> %0.10e </last_slope>\n", value);
                 }
-                ivalue =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_null_count].u_cnt;
-                fprintf(out_file, "\t\t\t<nan_count> %lu </nan_count>\n",
-                        ivalue);
-                ivalue =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_last_null_count].u_cnt;
-                fprintf(out_file,
-                        "\t\t\t<last_nan_count> %lu </last_nan_count>\n",
-                        ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                ivalue = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_null_count].u_cnt;
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<nan_count> %lu </nan_count>\n", ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                ivalue = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_last_null_count].u_cnt;
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<last_nan_count> %lu </last_nan_count>\n", ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
                 break;
             case CF_SEASONAL:
             case CF_DEVSEASONAL:
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_seasonal].u_val;
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_seasonal].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file, "\t\t\t<seasonal> NaN </seasonal>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<seasonal> NaN </seasonal>\n");
                 } else {
-                    fprintf(out_file, "\t\t\t<seasonal> %0.10e </seasonal>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<seasonal> %0.10e </seasonal>\n", value);
                 }
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_last_seasonal].u_val;
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_last_seasonal].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file,
-                            "\t\t\t<last_seasonal> NaN </last_seasonal>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_seasonal> NaN </last_seasonal>\n");
                 } else {
-                    fprintf(out_file,
-                            "\t\t\t<last_seasonal> %0.10e </last_seasonal>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_seasonal> %0.10e </last_seasonal>\n", value);
                 }
-                ivalue =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_init_seasonal].u_cnt;
-                fprintf(out_file, "\t\t\t<init_flag> %lu </init_flag>\n",
-                        ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                ivalue = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_init_seasonal].u_cnt;
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<init_flag> %lu </init_flag>\n", ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
                 break;
             case CF_DEVPREDICT:
                 break;
@@ -359,13 +443,16 @@
                                                                     ds_cnt +
                                                                     ii].
                                                        scratch);
-                fprintf(out_file, "\t\t\t<history> ");
+                cb("\t\t\t<history> ",
+                    strlen("\t\t\t<history> "), user);
                 for (vidx = 0;
-                     vidx < rrd.rra_def[i].par[RRA_window_len].u_cnt;
-                     ++vidx) {
-                    fprintf(out_file, "%d", violations_array[vidx]);
+                    vidx < rrd.rra_def[i].par[RRA_window_len].u_cnt;
+                    ++vidx) {
+                    snprintf(somestring_buf, 255, "%d", violations_array[vidx]);
+                    cb(somestring_buf, strlen(somestring_buf), user);
                 }
-                fprintf(out_file, " </history>\n");
+                cb(" </history>\n",
+                    strlen(" </history>\n"), user);
             }
                 break;
             case CF_AVERAGE:
@@ -373,26 +460,31 @@
             case CF_MINIMUM:
             case CF_LAST:
             default:
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_val].u_val;
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].scratch[CDP_val].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file, "\t\t\t<value> NaN </value>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<value> NaN </value>\n");
                 } else {
-                    fprintf(out_file, "\t\t\t<value> %0.10e </value>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<value> %0.10e </value>\n", value);
                 }
-                fprintf(out_file,
-                        "\t\t\t<unknown_datapoints> %lu </unknown_datapoints>\n",
-                        rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                     ii].scratch[CDP_unkn_pdp_cnt].u_cnt);
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<unknown_datapoints> %lu </unknown_datapoints>\n",
+                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                        scratch[CDP_unkn_pdp_cnt].u_cnt);
+                cb(somestring_buf, strlen(somestring_buf), user);
                 break;
             }
-            fprintf(out_file, "\t\t\t</ds>\n");
+            cb("\t\t\t</ds>\n",
+                strlen("\t\t\t</ds>\n"), user);
         }
-        fprintf(out_file, "\t\t</cdp_prep>\n");
+        cb("\t\t</cdp_prep>\n",
+            strlen("\t\t</cdp_prep>\n"), user);

-        fprintf(out_file, "\t\t<database>\n");
+        cb("\t\t<database>\n",
+            strlen("\t\t<database>\n"), user);
         rrd_seek(rrd_file, (rra_start + (rrd.rra_ptr[i].cur_row + 1)
                             * rrd.stat_head->ds_cnt
                             * sizeof(rrd_value_t)), SEEK_SET);
@@ -412,36 +504,75 @@
             timer++;
 #if HAVE_STRFTIME
             localtime_r(&now, &tm);
-            strftime(somestring, 200, "%Y-%m-%d %H:%M:%S %Z", &tm);
+            strftime(somestring, 255, "%Y-%m-%d %H:%M:%S %Z", &tm);
 #else
 # error "Need strftime"
 #endif
-            fprintf(out_file, "\t\t\t<!-- %s / %d --> <row>", somestring,
-                    (int) now);
+            snprintf(somestring_buf, 255,
+                "\t\t\t<!-- %s / %d --> <row>",  somestring, (int) now);
+            cb(somestring_buf, strlen(somestring_buf), user);
             for (iii = 0; iii < rrd.stat_head->ds_cnt; iii++) {
                 rrd_read(rrd_file, &my_cdp, sizeof(rrd_value_t) * 1);
                 if (isnan(my_cdp)) {
-                    fprintf(out_file, "<v> NaN </v>");
+                    snprintf(somestring_buf, 255, "<v> NaN </v>");
                 } else {
-                    fprintf(out_file, "<v> %0.10e </v>", my_cdp);
-                };
+                    snprintf(somestring_buf, 255, "<v> %0.10e </v>", my_cdp);
+                }
+                cb(somestring_buf, strlen(somestring_buf), user);
             }
-            fprintf(out_file, "</row>\n");
+            cb("</row>\n",
+                strlen("</row>\n"), user);
         }
-        fprintf(out_file, "\t\t</database>\n\t</rra>\n");
+        cb("\t\t</database>\n\t</rra>\n",
+            strlen("\t\t</database>\n\t</rra>\n"), user);
+    }

-    }
-    fprintf(out_file, "</rrd>\n");
+    cb("</rrd>\n",
+        strlen("</rrd>\n"), user);
+
     rrd_free(&rrd);
-    if (out_file != stdout) {
-        fclose(out_file);
-    }
+
 #ifdef HAVE_SETLOCALE
     setlocale(LC_NUMERIC, old_locale);
 #endif
+
     return rrd_close(rrd_file);
 }

+size_t rrd_dump_opt_cb_fileout(
+    const void *data,
+    size_t len,
+    void *user)
+{
+    return fwrite(data, 1, len, (FILE *)user);
+}
+
+int rrd_dump_opt_r(
+    const char *filename,
+    char *outname,
+    int opt_noheader)
+{
+    FILE     *out_file;
+    int       res;
+
+    out_file = NULL;
+    if (outname) {
+        if (!(out_file = fopen(outname, "w"))) {
+            return (-1);
+        }
+    } else {
+        out_file = stdout;
+    }
+
+    res = rrd_dump_opt_cb_r(filename, opt_noheader, rrd_dump_opt_cb_fileout, (void *)out_file);
+
+    if (out_file != stdout) {
+        fclose(out_file);
+    }
+
+    return res;
+}
+
 /* backward compatibility with 1.2.x */
 int rrd_dump_r(
     const char *filename,
Index: Z:/data/programming/own/rrdtool/trunk/lib/src/rrd.h
===================================================================
--- Z:/data/programming/own/rrdtool/trunk/lib/src/rrd.h (Revision 19)
+++ Z:/data/programming/own/rrdtool/trunk/lib/src/rrd.h (Revision 23)
@@ -122,6 +122,10 @@
         struct rrd_info_t *next;
     } rrd_info_t;

+    typedef size_t (* rrd_output_callback_t)(
+    const void *,
+    size_t,
+    void *);

 /* main function blocks */
     int       rrd_create(
@@ -243,6 +247,12 @@
     const char *filename,
     int rraindex);

+    int rrd_dump_opt_cb_r(
+    const char *filename,
+    int opt_noheader,
+    rrd_output_callback_t cb,
+    void *user);
+
 /* Transplanted from rrd_parsetime.h */
     typedef enum {
         ABSOLUTE_TIME,
Index: Z:/data/programming/own/rrdtool/trunk/lib/CONTRIBUTORS
===================================================================
--- Z:/data/programming/own/rrdtool/trunk/lib/CONTRIBUTORS (Revision 19)
+++ Z:/data/programming/own/rrdtool/trunk/lib/CONTRIBUTORS (Revision 23)
@@ -7,6 +7,7 @@
 Amos Shapira <amos with gezernet.co.il>
 Andreas Kroomaa <andre with ml.ee>
 Andrew Turner <turner with mint.net> (LAST consolidator)
+Benny Baumann <benbe with geshi.org) rrd_dump with callback support
 Bernard Fischer <bfischer with syslog.ch> 64bit stuff, --alt-autoscale-max
 Bernhard Fischer <rep dot dot dot nop with gmail.com> MMAP rewrite
 Bill Fenner <fenner with research.att.com>




_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (850 bytes) Download Attachment
Dan Cech-2

Re: Suggestion for API extension

Reply Threaded More More options
Print post
Permalink
Hi Benny,

Benny Baumann wrote:
> I recently discovered the PHP extension for RRDTool which is available
> as a contribution to RRDTool. This extension runs successfully with
> latest PHP5, but misses some functions of current RRDTool releases (no
> rrd_dump, no rrd_info). I'm currently working on updating the PHP
> extension to also include these operations.

Funny, I've been doing some work on the PHP rrdtool extension myself in
the last couple of weeks.  It's not quite ready for prime-time yet, but
I have completed the following:

- remove requirement to send argument count to rrd_graph
- add support for rrd_xport
- remove requirement to send argument count to rrd_fetch
- support multiple update values in rrd_update
- support rrd_lastupdate
- support rrd_first
- clean up phpinfo() output

I was planning to tackle rrd_info next, but haven't looked at rrd_dump.

--
Dan Cech
Ubersmith, Inc.
http://www.ubersmith.com

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Benny Baumann

Re: Suggestion for API extension

Reply Threaded More More options
Print post
Permalink
Hi,

Am 30.07.2009 15:18, schrieb Dan Cech:

> Hi Benny,
>
> Benny Baumann wrote:
>  
>> I recently discovered the PHP extension for RRDTool which is available
>> as a contribution to RRDTool. This extension runs successfully with
>> latest PHP5, but misses some functions of current RRDTool releases (no
>> rrd_dump, no rrd_info). I'm currently working on updating the PHP
>> extension to also include these operations.
>>    
>
> Funny, I've been doing some work on the PHP rrdtool extension myself in
> the last couple of weeks.  It's not quite ready for prime-time yet, but
> I have completed the following:
>
> - remove requirement to send argument count to rrd_graph
> - remove requirement to send argument count to rrd_fetch
>  
Sounds good.
> - add support for rrd_xport
>  
That is really good ;-)
> - support multiple update values in rrd_update
>  
Will need to have a look at this, but in general this sounds positive ;-)
> - support rrd_lastupdate
> - support rrd_first
>  
Great ;-)
> - clean up phpinfo() output
>  
Yep. That one was really messy :P

Though rrdtool_info will need some cleanup too. Simply returning an
array will all the needed values should be sufficiant and much more
practical.
> I was planning to tackle rrd_info next, but haven't looked at rrd_dump.
>  
I already completed rrd_info, but didn't parse the returned key names.
I'm writing a PHP wrapper for this PHP extension and as such I can do
parsing of the keynames there.

Some work on Open Basedir Restriction can be found at
http://blog.benny-baumann.de/?p=352 (German). The patch there isn't
fully up-to-date, bbut basically works (except for one minor typo I
already fixed in my local dev version).

Would be nice if we could exchange our versions to merge the changes,
thus avoiding duplicate work.

One problem (I mentioned in my blog) I couldn't solve yet is enforcing
the OpenBasedir Restriction for filenames mentioned in the command line
arguments (e.g. for rrd_graph. IDK if you've got a solution.

But well, back to the API extension suggested here:
What do you think of offering a callback based variant in addition to a
file based variant thus programs that need the returned data in a buffer
can grab it without needing to write to the filesystem?

Other functions (e.g. for rrd_graph) that write to disk might use such a
callback too, or offer returning the ressources they created to the
caller (i.e. giving the generated image as a internal ressource to the
caller).

Regards,
BenBE.



_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (850 bytes) Download Attachment
Dan Cech-2

Re: Suggestion for API extension

Reply Threaded More More options
Print post
Permalink
Benny Baumann wrote:
> Though rrdtool_info will need some cleanup too. Simply returning an
> array will all the needed values should be sufficiant and much more
> practical.

This is an interesting BC question, all the updates I've made so far are
completely backwards compatible, not requiring any changes to existing
code using the extension.  Changing the rrdtool_info function to return
an array rather than printing the information is a pretty major change.

The rrd_fetch output presents a similar problem, all the values are
returned in a single long array at present.  It would be nice to return
the data as a multi-dimensional array with each 'row' of data grouped
together (and very simple to do in the extension) but it would break BC
with existing scripts using the current version of this function.

>> I was planning to tackle rrd_info next, but haven't looked at rrd_dump.
>>  
> I already completed rrd_info, but didn't parse the returned key names.
> I'm writing a PHP wrapper for this PHP extension and as such I can do
> parsing of the keynames there.

Yeah, returning the raw data as it comes from rrd_info_r is easy, but I
was planning to parse it out into a multi-dimensional php array.  A
separate php-code library to parse the output of the rrd_info function
in the php extension seems like it would make it more difficult to use
the system because you would need to have the userland php code as well
as the extension itself.

> Some work on Open Basedir Restriction can be found at
> http://blog.benny-baumann.de/?p=352 (German). The patch there isn't
> fully up-to-date, bbut basically works (except for one minor typo I
> already fixed in my local dev version).

I hadn't considered open_basedir, but it seems like a good idea to add
this safety feature.  Enforcing open_basedir on the actual RRDs
referenced in graph, xport, etc will be more difficult though!

> Would be nice if we could exchange our versions to merge the changes,
> thus avoiding duplicate work.

Agreed, any suggestions on the best way to do that?

> What do you think of offering a callback based variant in addition to a
> file based variant thus programs that need the returned data in a buffer
> can grab it without needing to write to the filesystem?
>
> Other functions (e.g. for rrd_graph) that write to disk might use such a
> callback too, or offer returning the ressources they created to the
> caller (i.e. giving the generated image as a internal ressource to the
> caller).

I can see how this might be a useful thing to support, in the case of
rrd_graph I was considering implementing and interface to graphv which
would allow returning the image data as a php string directly, but the
volume of data returned by rrd_dump can obviously be much larger than
the typical graph image.

I typically output the graph images to temporary files named according
to a hash of the graph arguments, which allows me to re-use graph images
for multiple clients without needing to regenerate them from the source
rrds (very useful when multiple clients are requesting identical graphs).

I'm no expert but it seems like the data will still all need to be
collected into a buffer before it can be returned to the calling php
script as a string or php array, so in the case of rrd_dump it may be
more efficient to write out the xml to disk and perform any operations
on it using a stream-based xml parser.

Dan

--
Dan Cech
Ubersmith, Inc.
http://www.ubersmith.com

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Benny Baumann

Re: Suggestion for API extension

Reply Threaded More More options
Print post
Permalink
Am 30.07.2009 17:46, schrieb Dan Cech:

> Benny Baumann wrote:
>  
>> Though rrdtool_info will need some cleanup too. Simply returning an
>> array will all the needed values should be sufficiant and much more
>> practical.
>>    
>
> This is an interesting BC question, all the updates I've made so far are
> completely backwards compatible, not requiring any changes to existing
> code using the extension.  Changing the rrdtool_info function to return
> an array rather than printing the information is a pretty major change.
>  
I more or less thought of a parameter $as_array = false which if not
given defaults to false, if given tells whether the string should be
given or an array with all the information should be returned.

But since I doubt this extension in the old form has reached any
significant distribution even having tthe array being returned as
default shouldn't matter that much.
> The rrd_fetch output presents a similar problem, all the values are
> returned in a single long array at present.  It would be nice to return
> the data as a multi-dimensional array with each 'row' of data grouped
> together (and very simple to do in the extension) but it would break BC
> with existing scripts using the current version of this function.
>  
As the existing extension is quite incomplete I guess noone should
really complain about if it is more or less rewritten and thus modified
to be of much more use. Even then I'm writing on a Library wrapper in
PHP that will take care of wrapping the functional interface of the
extension into an OOP interface for PHP5. More on this wrapper library
once the basic usage works properly.
>>> I was planning to tackle rrd_info next, but haven't looked at rrd_dump.  
>>>      
>> I already completed rrd_info, but didn't parse the returned key names.
>> I'm writing a PHP wrapper for this PHP extension and as such I can do
>> parsing of the keynames there.
>>    
> Yeah, returning the raw data as it comes from rrd_info_r is easy, but I
> was planning to parse it out into a multi-dimensional php array.  A
>  
I planned to do this in my wrapper library as I have to postprocess the
data anyways.
> separate php-code library to parse the output of the rrd_info function
> in the php extension seems like it would make it more difficult to use
> the system because you would need to have the userland php code as well
> as the extension itself.
>  
The userland code is a wrapper around the functions in such a way that
you won't need to care about the command line interface for RRDTool. For
example, even though the creation of a new RRD file is much more
verbose, the following source saves you from doing all the calculations
yourself:

---
include 'rrdtool.php';

$RRD = RRDTool::Archive();
if(!$RRD->Load('net.rrd')) {
    $RRD->Start = 0;
   
    $RRD->addDS(
        RRDTool_DataSource::createDS(
            'trafficIn', RRD_DS_COUNTER, 600,
            array(
                RRD_PARAM_CF_MIN => 0,
                RRD_PARAM_CF_MAX => 65535
                )
            )
        );
    $RRD->addDS(
        RRDTool_DataSource::createDS(
            'trafficOut', RRD_DS_COUNTER, 600,
            array(
                RRD_PARAM_CF_MIN => 0,
                RRD_PARAM_CF_MAX => 65535
                )
            )
        );

    $RRD->addRRA(RRDTool_Archive::createRRACF(
        RRD_RRA_CF_AVG, 0.5,
        $RRD->getPDPIC(RRD_DEFAULT_STEP),
        $RRD->getPDPIC(RRD_TIME_DAY)
        ));
    $RRD->addRRA(RRDTool_Archive::createRRACF(
        RRD_RRA_CF_AVG, 0.5,
        $RRD->getPDPIC(15 * RRD_TIME_MINUTE),
        $RRD->getPDPIC(RRD_TIME_WEEK, 15 * RRD_TIME_MINUTE)
        ));
    $RRD->addRRA(RRDTool_Archive::createRRACF(
        RRD_RRA_CF_AVG, 0.5,
        $RRD->getPDPIC(RRD_TIME_HOUR),
        $RRD->getPDPIC(RRD_TIME_MONTH, RRD_TIME_HOUR)
        ));
    $RRD->addRRA(RRDTool_Archive::createRRACF(
        RRD_RRA_CF_AVG, 0.5,
        $RRD->getPDPIC(4 * RRD_TIME_HOUR),
        $RRD->getPDPIC(RRD_TIME_YEAR, 4 * RRD_TIME_HOUR)
        ));

    $RRD->addRRA(RRDTool_Archive::createRRACF(
        RRD_RRA_CF_MAX, 0.5,
        $RRD->getPDPIC(15 * RRD_TIME_MINUTE),
        $RRD->getPDPIC(RRD_TIME_DAY, 15 * RRD_TIME_MINUTE)
        ));
    $RRD->addRRA(RRDTool_Archive::createRRACF(
        RRD_RRA_CF_MAX, 0.5,
        $RRD->getPDPIC(RRD_TIME_HOUR),
        $RRD->getPDPIC(RRD_TIME_WEEK, RRD_TIME_HOUR)
        ));
    $RRD->addRRA(RRDTool_Archive::createRRACF(
        RRD_RRA_CF_MAX, 0.5,
        $RRD->getPDPIC(4 * RRD_TIME_HOUR),
        $RRD->getPDPIC(RRD_TIME_MONTH, 4 * RRD_TIME_HOUR)
        ));
    $RRD->addRRA(RRDTool_Archive::createRRACF(
        RRD_RRA_CF_MAX, 0.5,
        $RRD->getPDPIC(12 * RRD_TIME_HOUR),
        $RRD->getPDPIC(RRD_TIME_YEAR, 12 * RRD_TIME_HOUR)
        ));

    $RRD->Save('net.rrd');
}
---

The rrdtool.php is still in developement and thus without any real
function yet (I needed the rrd_info function first for reading back
structural information).

What the above code will create should be obvious. As I said: It's a bit
more verbose, but much more logical to the causal observer. The Function
getPDPIC stands for Primary Data Point Interval Calculation ... What it
does should be obvious :P
>> Some work on Open Basedir Restriction can be found at
>> http://blog.benny-baumann.de/?p=352 (German). The patch there isn't
>> fully up-to-date, bbut basically works (except for one minor typo I
>> already fixed in my local dev version).
>>    
> I hadn't considered open_basedir, but it seems like a good idea to add
> this safety feature.  Enforcing open_basedir on the actual RRDs
> referenced in graph, xport, etc will be more difficult though!
>  
Yes. Correct, though this might require some parsing on the parameters
given. I spared that point for now though as xport and graph don't write
to files referenced in the parameter strings, yet only to the filename
given as target. And as RRD rejects files not confirming to its
fileformat no real harm should be possible (You only might compromise
existing RRDs if you know where they are).

But completing the Basedir Restriction should be included in a final
version of the extension. That's why I emphasized this in my blog
posting, as the current implementation leaves some holes people should
be aware of when using this extension.
>> Would be nice if we could exchange our versions to merge the changes,
>> thus avoiding duplicate work.
>>    
> Agreed, any suggestions on the best way to do that?
>  
I have my version in a SVN repo. You either could:
1. Send me your file (or attach it) and I'll merge it into my SVN,
sending back the result
OR
2. Send me a username+APR1 password hash (htpasswd) and I give you RW
access on that repository.
OR
3. We use a completely different approach :P

>> What do you think of offering a callback based variant in addition to a
>> file based variant thus programs that need the returned data in a buffer
>> can grab it without needing to write to the filesystem?
>>
>> Other functions (e.g. for rrd_graph) that write to disk might use such a
>> callback too, or offer returning the ressources they created to the
>> caller (i.e. giving the generated image as a internal ressource to the
>> caller).
>>    
> I can see how this might be a useful thing to support, in the case of
> rrd_graph I was considering implementing and interface to graphv which
> would allow returning the image data as a php string directly, but the
> volume of data returned by rrd_dump can obviously be much larger than
> the typical graph image.
>  
IDK if it would be that hard to enable the php extension to return e.g.
a GD2 image ressource as its result. Though having it return the image
data as string shouldn't be wrong neither (IIRC GD supports loading
images from string input - and be it (ab)using some fopen wrapper magic).
> I typically output the graph images to temporary files named according
> to a hash of the graph arguments, which allows me to re-use graph images
> for multiple clients without needing to regenerate them from the source
> rrds (very useful when multiple clients are requesting identical graphs).
>  
I see. The downside (and I exactly have this issue on my server) is if
your HDD is connected by slow media (e.g. iSCSI) and thus reading from
or writing to disk takes considerable time, compared to allocating about
100k.
> I'm no expert but it seems like the data will still all need to be
> collected into a buffer before it can be returned to the calling php
> script as a string or php array, so in the case of rrd_dump it may be
> more efficient to write out the xml to disk and perform any operations
> on it using a stream-based xml parser.
>  
Well, given the callback you can essentially just do exactly this: Every
time you get a new line from rrd_dump you can feed it as next input to
the XML parser. Anyway for my implementation of rrd_dump I want the
extension function to return the output as a bare string - as if you
hade piped the output into a memory buffer. Using such a callback method
seems most reasonable to me as you should do as much as possible in
primary memory.
> Dan
>  
Regards,
BenBE.



_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (850 bytes) Download Attachment
oetiker

Re: Suggestion for API extension

Reply Threaded More More options
Print post
Permalink
In reply to this post by Benny Baumann
Hi Benny,

Jul 30 Benny Baumann wrote:

> Hi,
>
> I recently discovered the PHP extension for RRDTool which is available
> as a contribution to RRDTool. This extension runs successfully with
> latest PHP5, but misses some functions of current RRDTool releases (no
> rrd_dump, no rrd_info). I'm currently working on updating the PHP
> extension to also include these operations.
>
> And here comes the issue: The RRDTool library is more or less written to
> be used in the command line interface thus either returns only single
> values or dumps output to a file\terminal. For the PHP extension I need
> output to be written into a buffer though.
>
> To solve this I suggest adding a callback interface to e.g. rrd_dump
> that is called whenever string data (or binary data) has to be returned.
> An example implementation for rrd_dump_opt_cb_r can be found in the
> attached patch. Feedback appreciated. IDK if this patch is complete and
> if the mentioned rrd_dump_opt_cb_r function is properly exported; but at
> least dumping should work.
>
> The basic rrd_dump_opt_r now calls this Callback method internally to
> give an example of how this callback interface can be used and to avoid
> duplicate source.

the patch seems sane to me from looking at your code ... I will be
glad to include it if you can provide a version that applies to

  svn://svn.oetiker.ch/rrdtool/trunk/program

also you should add a section to doc/librrd.pod explaining this new
access method ...

cheers
tobi

>
> Regards,
> BenBE.
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Yann JOUANIN

RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
Hello list,

It seems the behavior of RRD client when using RRDCacheD can make trouble
when using RRDCacheD on another host than the one where using client.

The translation between relative path and absolute path is done in client
code (rrdc_flush) while it should only be done by the server.

This causes update to fail because the path is badly rewritten.
I had to comment the following line to fix this.



int rrdc_flush (const char *filename) /* {{{ */
{
.........


  /* change to absolute path for rrdcached */
/*  if (*filename != '/' && realpath(filename, file_path) != NULL)
      filename = file_path; */
............



Yann Jouanin

http://www.yannj.fr



_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
oetiker

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
Hi Yann,

Today Yann Jouanin wrote:

> Hello list,
>
> It seems the behavior of RRD client when using RRDCacheD can make trouble
> when using RRDCacheD on another host than the one where using client.
>
> The translation between relative path and absolute path is done in client
> code (rrdc_flush) while it should only be done by the server.
>
> This causes update to fail because the path is badly rewritten.
> I had to comment the following line to fix this.
>
>
>
> int rrdc_flush (const char *filename) /* {{{ */
> {
> .........
>
>
>   /* change to absolute path for rrdcached */
> /*  if (*filename != '/' && realpath(filename, file_path) != NULL)
>       filename = file_path; */
> ............

Yann, can you elaborate how you would handle relative path names on
the server side ?

cheers
tobi



--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Yann JOUANIN

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
Isn't it already done by this function in RRDCacheD and defining base dir?

/* when using a base dir, convert relative paths to absolute paths.
* if necessary, modifies the "filename" pointer to point
* to the new path created in "tmp".  "tmp" is provided
* by the caller and sizeof(tmp) must be >= PATH_MAX.
*
* this allows us to optimize for the expected case (absolute path)
* with a no-op.
*/
1059 static void get_abs_path(char **filename, char *tmp)

Yann Jouanin

http://www.yannj.fr


-----Original Message-----
From: Tobias Oetiker [mailto:[hidden email]]
Sent: Saturday, August 08, 2009 4:19 PM
To: Yann Jouanin
Cc: [hidden email]
Subject: Re: [rrd-developers] RRDCacheD - Client rewriting path

Hi Yann,

Today Yann Jouanin wrote:

> Hello list,
>
> It seems the behavior of RRD client when using RRDCacheD can make trouble
> when using RRDCacheD on another host than the one where using client.
>
> The translation between relative path and absolute path is done in client
> code (rrdc_flush) while it should only be done by the server.
>
> This causes update to fail because the path is badly rewritten.
> I had to comment the following line to fix this.
>
>
>
> int rrdc_flush (const char *filename) /* {{{ */
> {
> .........
>
>
>   /* change to absolute path for rrdcached */
> /*  if (*filename != '/' && realpath(filename, file_path) != NULL)
>       filename = file_path; */
> ............

Yann, can you elaborate how you would handle relative path names on
the server side ?

cheers
tobi



--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Benny Baumann

Re: [PATCH] Suggestion for API extension (rrd_dump)

Reply Threaded More More options
Print post
Permalink
In reply to this post by oetiker
Hi Tobias,

Am 08.08.2009 11:49, schrieb Tobias Oetiker:
> Hi Benny,
>
> the patch seems sane to me from looking at your code ... I will be
> glad to include it if you can provide a version that applies to
>
>   svn://svn.oetiker.ch/rrdtool/trunk/program
>  
As the version available in the public download directory and the SVN
differ quite a bit, I had some work on merging in my changes, thus you
might notice some differences between the old and the new API extension
patch. In particular I chose to rename the API function from
rrd_dump_opt_cb_r to rrd_dump_cb_r. I furthermore hope I didn't miss any
changes made in the SVN when I merged the files.
> also you should add a section to doc/librrd.pod explaining this new
> access method ...
>  
The API documentation explains the usage of this interface using the
internally used rrd_dump_opt_cb_fileout callback which should, because
of its simplicity, be a well-enough example.
> cheers
> tobi
>  
IDK if the rrd_dump_opt_r function should be exported too (and not only
used internally wrapped by rrd_dump and rrd_dump_r).?

Anyway: The working patch, based on revision 1886 is attached. If there
are any questions left, feel free to contact me.

Regards,
BenBE.

[rrd_dump_cb_r.patch]

Index: CONTRIBUTORS
===================================================================
--- CONTRIBUTORS (Revision 1886)
+++ CONTRIBUTORS (Arbeitskopie)
@@ -9,6 +9,7 @@
 Amos Shapira <amos with gezernet.co.il>
 Andreas Kroomaa <andre with ml.ee>
 Andrew Turner <turner with mint.net> (LAST consolidator)
+Benny Baumann <benbe with geshi.org) rrd_dump with callback support
 Bernard Fischer <bfischer with syslog.ch> 64bit stuff, --alt-autoscale-max
 Bernhard Fischer <rep dot dot dot nop with gmail.com> MMAP rewrite
 Bill Fenner <fenner with research.att.com>
Index: doc/librrd.pod
===================================================================
--- doc/librrd.pod (Revision 1886)
+++ doc/librrd.pod (Arbeitskopie)
@@ -58,4 +58,41 @@
     rrd_free_ptrs(&arr, &arr_size);
     /* here, arr == NULL && arr_size == 0 */
 
+=item B<rrd_dump_cr_r(char *filename, int opt_header, rrd_output_callback_t cb, void *user)>
+
+In some situations it is necessary to get the output of C<rrd_dump> without
+writing it to a file or the standard output. In such cases an application
+can ask B<rrd_dump_cb_r> to call an user-defined function each time there
+is output to be stored somewhere. This can be used, to e.g. directly feed
+an XML parser with the dumped output or transfer the resulting string
+in memory.
+
+The arguments for B<rrd_dump_cb_r> are the same as for B<rrd_dump_opt_r>
+except that the output filename parameter is replaced by the user-defined
+callback function and an additional parameter for the callback function
+that is passed untouched, i.e. to store information about the callback state
+needed for the user-defined callback to function properly.
+
+Recent versions of B<rrd_dump_opt_r> internally use this callback mechanism
+to write their output to the file provided by the user.
+
+    size_t rrd_dump_opt_cb_fileout(
+        const void *data,
+        size_t len,
+        void *user)
+    {
+        return fwrite(data, 1, len, (FILE *)user);
+    }
+
+The associated call for B<rrd_dump_cb_r> looks like
+
+    res = rrd_dump_cb_r(filename, opt_header,
+        rrd_dump_opt_cb_fileout, (void *)out_file);
+
+where the last parameter specifies the file handle B<rrd_dump_opt_cb_fileout>
+should write to. There's no specific condition for the callback to detect
+when it is called for the first time, nor for the last time. If you require
+this for initialization and cleanup you should do those tasks before and
+after calling B<rrd_dump_cr_r> respectively.
+
 =back
Index: src/rrd.h
===================================================================
--- src/rrd.h (Revision 1886)
+++ src/rrd.h (Arbeitskopie)
@@ -130,6 +130,10 @@
         struct rrd_info_t *next;
     } rrd_info_t;
 
+    typedef size_t (* rrd_output_callback_t)(
+    const void *,
+    size_t,
+    void *);
 
 /* main function blocks */
     int       rrd_create(
@@ -252,6 +256,12 @@
     const char *filename,
     int rraindex);
 
+    int rrd_dump_cb_r(
+    const char *filename,
+    int opt_header,
+    rrd_output_callback_t cb,
+    void *user);
+
 /* Transplanted from rrd_parsetime.h */
     typedef enum {
         ABSOLUTE_TIME,
Index: src/rrd_dump.c
===================================================================
--- src/rrd_dump.c (Revision 1886)
+++ src/rrd_dump.c (Arbeitskopie)
@@ -49,123 +49,172 @@
 #include <locale.h>
 #endif
 
-
 #if !(defined(NETWARE) || defined(WIN32))
 extern char *tzname[2];
 #endif
 
-static int rrd_dump_opt_r(
+//Local prototypes
+size_t rrd_dump_opt_cb_fileout(
+    const void *data,
+    size_t len,
+    void *user);
+
+
+int rrd_dump_cb_r(
     const char *filename,
-    char *outname,
-    int opt_header)
+    int opt_header,
+    rrd_output_callback_t cb,
+    void *user)
 {
     unsigned int i, ii, ix, iii = 0;
     time_t    now;
     char      somestring[255];
+    char      somestring_buf[255];
     rrd_value_t my_cdp;
     off_t     rra_base, rra_start, rra_next;
     rrd_file_t *rrd_file;
-    FILE     *out_file;
     rrd_t     rrd;
     rrd_value_t value;
     struct tm tm;
     char *old_locale = "";
 
+    //Check if we got a (valid) callback method
+    if (!cb) {
+        return (-1);
+    }
+
     rrd_init(&rrd);
+
     rrd_file = rrd_open(filename, &rrd, RRD_READONLY | RRD_READAHEAD);
     if (rrd_file == NULL) {
         rrd_free(&rrd);
         return (-1);
     }
 
-    out_file = NULL;
-    if (outname) {
-        if (!(out_file = fopen(outname, "w"))) {
-            return (-1);
-        }
-    } else {
-        out_file = stdout;
-    }
 #ifdef HAVE_SETLOCALE
     old_locale = setlocale(LC_NUMERIC, "C");
 #endif
 
     if (opt_header == 1) {
-        fputs("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n", out_file);
-        fputs
-            ("<!DOCTYPE rrd SYSTEM \"http://oss.oetiker.ch/rrdtool/rrdtool.dtd\">\n",
-             out_file);
- fputs("<!-- Round Robin Database Dump -->\n", out_file);
- fputs("<rrd>\n", out_file);
+        cb("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n",
+            strlen("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"), user);
+        cb("<!DOCTYPE rrd SYSTEM \"http://oss.oetiker.ch/rrdtool/rrdtool.dtd\">\n",
+            strlen("<!DOCTYPE rrd SYSTEM \"http://oss.oetiker.ch/rrdtool/rrdtool.dtd\">\n"), user);
+        cb("<!-- Round Robin Database Dump -->\n",
+            strlen("<!-- Round Robin Database Dump -->\n"), user);
+        cb("<rrd>\n",
+            strlen("<rrd>\n"), user);
     } else if (opt_header == 2) {
-     fputs("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n", out_file);
- fputs("<!-- Round Robin Database Dump -->\n", out_file);
- fputs("<rrd xmlns=\"http://oss.oetiker.ch/rrdtool/rrdtool-dump.xml\" xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n", out_file);
- fputs("\txsi:schemaLocation=\"http://oss.oetiker.ch/rrdtool/rrdtool-dump.xml http://oss.oetiker.ch/rrdtool/rrdtool-dump.xsd\">\n", out_file);
+        cb("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n",
+            strlen("<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"), user);
+        cb("<!-- Round Robin Database Dump -->\n",
+            strlen("<!-- Round Robin Database Dump -->\n"), user);
+        cb("<rrd xmlns=\"http://oss.oetiker.ch/rrdtool/rrdtool-dump.xml\" xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n",
+            strlen("<rrd xmlns=\"http://oss.oetiker.ch/rrdtool/rrdtool-dump.xml\" xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n"), user);
+        cb("\txsi:schemaLocation=\"http://oss.oetiker.ch/rrdtool/rrdtool-dump.xml http://oss.oetiker.ch/rrdtool/rrdtool-dump.xsd\">\n",
+            strlen("\txsi:schemaLocation=\"http://oss.oetiker.ch/rrdtool/rrdtool-dump.xml http://oss.oetiker.ch/rrdtool/rrdtool-dump.xsd\">\n"), user);
     } else {
-     fputs("<!-- Round Robin Database Dump -->\n", out_file);
-     fputs("<rrd>\n", out_file);
+        cb("<!-- Round Robin Database Dump -->\n",
+            strlen("<!-- Round Robin Database Dump -->\n"), user);
+        cb("<rrd>\n",
+            strlen("<rrd>\n"), user);
     }
 
+    cb("<!-- Round Robin Database Dump -->",
+        strlen("<!-- Round Robin Database Dump -->"), user);
+
+    cb("<rrd>",
+        strlen("<rrd>"), user);
+
     if (atoi(rrd.stat_head->version) <= 3) {
-        fprintf(out_file, "\t<version>%s</version>\n", RRD_VERSION3);
+        snprintf(somestring, 255, "\t<version>%s</version>\n", RRD_VERSION3);
     } else {
-        fprintf(out_file, "\t<version>%s</version>\n", RRD_VERSION);
+        snprintf(somestring, 255, "\t<version>%s</version>\n", RRD_VERSION);
     }
-    fprintf(out_file, "\t<step>%lu</step> <!-- Seconds -->\n",
-            rrd.stat_head->pdp_step);
+    cb(somestring, strlen(somestring), user);
+    
+    snprintf(somestring, 255, "\t<step>%lu</step> <!-- Seconds -->\n",
+        rrd.stat_head->pdp_step);
+    cb(somestring, strlen(somestring), user);
+
 #ifdef HAVE_STRFTIME
     localtime_r(&rrd.live_head->last_up, &tm);
-    strftime(somestring, 200, "%Y-%m-%d %H:%M:%S %Z", &tm);
+    strftime(somestring, 255, "%Y-%m-%d %H:%M:%S %Z", &tm);
 #else
 # error "Need strftime"
 #endif
-    fprintf(out_file, "\t<lastupdate>%lld</lastupdate> <!-- %s -->\n\n",
-            (long long) rrd.live_head->last_up, somestring);
+    snprintf(somestring_buf, 255, "\t<lastupdate>%lld</lastupdate> <!-- %s -->\n\n",
+        (long long) rrd.live_head->last_up, somestring);
+    cb(somestring_buf, strlen(somestring_buf), user);
     for (i = 0; i < rrd.stat_head->ds_cnt; i++) {
-        fprintf(out_file, "\t<ds>\n");
-        fprintf(out_file, "\t\t<name>%s</name>\n", rrd.ds_def[i].ds_nam);
-        fprintf(out_file, "\t\t<type>%s</type>\n", rrd.ds_def[i].dst);
+        cb("\t<ds>\n",
+            strlen("\t<ds>\n"), user);
+
+        snprintf(somestring_buf, 255, "\t\t<name> %s </name>\n", rrd.ds_def[i].ds_nam);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
+        snprintf(somestring_buf, 255, "\t\t<type> %s </type>\n", rrd.ds_def[i].dst);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
         if (dst_conv(rrd.ds_def[i].dst) != DST_CDEF) {
-            fprintf(out_file,
-                    "\t\t<minimal_heartbeat>%lu</minimal_heartbeat>\n",
+            snprintf(somestring_buf, 255, "\t\t<minimal_heartbeat>%lu</minimal_heartbeat>\n",
                     rrd.ds_def[i].par[DS_mrhb_cnt].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             if (isnan(rrd.ds_def[i].par[DS_min_val].u_val)) {
-                fprintf(out_file, "\t\t<min>NaN</min>\n");
+                snprintf(somestring_buf, 255, "\t\t<min>NaN</min>\n");
             } else {
-                fprintf(out_file, "\t\t<min>%0.10e</min>\n",
-                        rrd.ds_def[i].par[DS_min_val].u_val);
+                snprintf(somestring_buf, 255, "\t\t<min>%0.10e</min>\n",
+                    rrd.ds_def[i].par[DS_min_val].u_val);
             }
+            cb(somestring_buf, strlen(somestring_buf), user);
+            
             if (isnan(rrd.ds_def[i].par[DS_max_val].u_val)) {
-                fprintf(out_file, "\t\t<max>NaN</max>\n");
+                snprintf(somestring_buf, 255, "\t\t<max>NaN</max>\n");
             } else {
-                fprintf(out_file, "\t\t<max>%0.10e</max>\n",
-                        rrd.ds_def[i].par[DS_max_val].u_val);
+                snprintf(somestring_buf, 255, "\t\t<max>%0.10e</max>\n",
+                    rrd.ds_def[i].par[DS_max_val].u_val);
             }
+            cb(somestring_buf, strlen(somestring_buf), user);
         } else {        /* DST_CDEF */
             char     *str = NULL;
 
             rpn_compact2str((rpn_cdefds_t *) &(rrd.ds_def[i].par[DS_cdef]),
-                            rrd.ds_def, &str);
-            fprintf(out_file, "\t\t<cdef>%s</cdef>\n", str);
+                rrd.ds_def, &str);
+
+            //Splitting into 3 writes to avoid allocating memory
+            //This is better compared to snprintf as str may be of arbitrary size
+            cb("\t\t<cdef> ", strlen("\t\t<cdef>"), user);
+            cb(str, strlen(str), user);
+            cb(" </cdef>\n", strlen("</cdef>\n"), user);
+
             free(str);
         }
-        fprintf(out_file, "\n\t\t<!-- PDP Status -->\n");
-        fprintf(out_file, "\t\t<last_ds>%s</last_ds>\n",
-                rrd.pdp_prep[i].last_ds);
+
+        cb("\n\t\t<!-- PDP Status -->\n",
+            strlen("\n\t\t<!-- PDP Status -->\n"), user);
+        snprintf(somestring_buf, 255, "\t\t<last_ds>%s</last_ds>\n",
+            rrd.pdp_prep[i].last_ds);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
         if (isnan(rrd.pdp_prep[i].scratch[PDP_val].u_val)) {
-            fprintf(out_file, "\t\t<value>NaN</value>\n");
+            snprintf(somestring_buf, 255, "\t\t<value>NaN</value>\n");
         } else {
-            fprintf(out_file, "\t\t<value>%0.10e</value>\n",
-                    rrd.pdp_prep[i].scratch[PDP_val].u_val);
+            snprintf(somestring_buf, 255, "\t\t<value>%0.10e</value>\n",
+                rrd.pdp_prep[i].scratch[PDP_val].u_val);
         }
-        fprintf(out_file, "\t\t<unknown_sec>%lu</unknown_sec>\n",
-                rrd.pdp_prep[i].scratch[PDP_unkn_sec_cnt].u_cnt);
+        cb(somestring_buf, strlen(somestring_buf), user);
 
-        fprintf(out_file, "\t</ds>\n\n");
+        snprintf(somestring_buf, 255, "\t\t<unknown_sec> %lu </unknown_sec>\n",
+            rrd.pdp_prep[i].scratch[PDP_unkn_sec_cnt].u_cnt);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
+        cb("\t</ds>\n\n",
+            strlen("\t</ds>\n\n"), user);
     }
 
-    fputs("<!-- Round Robin Archives -->\n", out_file);
+    cb("<!-- Round Robin Archives -->\n",
+        strlen("<!-- Round Robin Archives -->\n"), user);
 
     rra_base = rrd_file->header_len;
     rra_next = rra_base;
@@ -177,182 +226,224 @@
         rra_start = rra_next;
         rra_next += (rrd.stat_head->ds_cnt
                      * rrd.rra_def[i].row_cnt * sizeof(rrd_value_t));
-        fprintf(out_file, "\t<rra>\n");
-        fprintf(out_file, "\t\t<cf>%s</cf>\n", rrd.rra_def[i].cf_nam);
-        fprintf(out_file,
-                "\t\t<pdp_per_row>%lu</pdp_per_row> <!-- %lu seconds -->\n\n",
-                rrd.rra_def[i].pdp_cnt,
-                rrd.rra_def[i].pdp_cnt * rrd.stat_head->pdp_step);
+
+        cb("\t<rra>\n",
+            strlen("\t<rra>\n"), user);
+
+        snprintf(somestring_buf, 255,
+            "\t\t<cf>%s</cf>\n", rrd.rra_def[i].cf_nam);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
+        snprintf(somestring_buf, 255,
+            "\t\t<pdp_per_row>%lu</pdp_per_row> <!-- %lu seconds -->\n\n",
+            rrd.rra_def[i].pdp_cnt,
+            rrd.rra_def[i].pdp_cnt * rrd.stat_head->pdp_step);
+        cb(somestring_buf, strlen(somestring_buf), user);
+
         /* support for RRA parameters */
-        fprintf(out_file, "\t\t<params>\n");
+        cb("\t\t<params>\n",
+            strlen("\t\t<params>\n"), user);
+
         switch (cf_conv(rrd.rra_def[i].cf_nam)) {
         case CF_HWPREDICT:
         case CF_MHWPREDICT:
-            fprintf(out_file, "\t\t<hw_alpha>%0.10e</hw_alpha>\n",
+            snprintf(somestring_buf, 255, "\t\t<hw_alpha>%0.10e</hw_alpha>\n",
                     rrd.rra_def[i].par[RRA_hw_alpha].u_val);
-            fprintf(out_file, "\t\t<hw_beta>%0.10e</hw_beta>\n",
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255, "\t\t<hw_beta>%0.10e</hw_beta>\n",
                     rrd.rra_def[i].par[RRA_hw_beta].u_val);
-            fprintf(out_file,
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255,
                     "\t\t<dependent_rra_idx>%lu</dependent_rra_idx>\n",
                     rrd.rra_def[i].par[RRA_dependent_rra_idx].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
             break;
         case CF_SEASONAL:
         case CF_DEVSEASONAL:
-            fprintf(out_file,
+            snprintf(somestring_buf, 255,
                     "\t\t<seasonal_gamma>%0.10e</seasonal_gamma>\n",
                     rrd.rra_def[i].par[RRA_seasonal_gamma].u_val);
-            fprintf(out_file,
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255,
                     "\t\t<seasonal_smooth_idx>%lu</seasonal_smooth_idx>\n",
                     rrd.rra_def[i].par[RRA_seasonal_smooth_idx].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             if (atoi(rrd.stat_head->version) >= 4) {
-                fprintf(out_file,
-                        "\t\t<smoothing_window>%0.10e</smoothing_window>\n",
-                        rrd.rra_def[i].par[RRA_seasonal_smoothing_window].
-                        u_val);
+                snprintf(somestring_buf, 255,
+                    "\t\t<smoothing_window>%0.10e</smoothing_window>\n",
+                    rrd.rra_def[i].par[RRA_seasonal_smoothing_window].u_val);
+                cb(somestring_buf, strlen(somestring_buf), user);
             }
-            fprintf(out_file,
+            snprintf(somestring_buf, 255,
                     "\t\t<dependent_rra_idx>%lu</dependent_rra_idx>\n",
                     rrd.rra_def[i].par[RRA_dependent_rra_idx].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
             break;
         case CF_FAILURES:
-            fprintf(out_file, "\t\t<delta_pos>%0.10e</delta_pos>\n",
+            snprintf(somestring_buf, 255, "\t\t<delta_pos>%0.10e</delta_pos>\n",
                     rrd.rra_def[i].par[RRA_delta_pos].u_val);
-            fprintf(out_file, "\t\t<delta_neg>%0.10e</delta_neg>\n",
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255, "\t\t<delta_neg>%0.10e</delta_neg>\n",
                     rrd.rra_def[i].par[RRA_delta_neg].u_val);
-            fprintf(out_file, "\t\t<window_len>%lu</window_len>\n",
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255, "\t\t<window_len>%lu</window_len>\n",
                     rrd.rra_def[i].par[RRA_window_len].u_cnt);
-            fprintf(out_file,
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            snprintf(somestring_buf, 255,
                     "\t\t<failure_threshold>%lu</failure_threshold>\n",
                     rrd.rra_def[i].par[RRA_failure_threshold].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             /* fall thru */
         case CF_DEVPREDICT:
-            fprintf(out_file,
+            snprintf(somestring_buf, 255,
                     "\t\t<dependent_rra_idx>%lu</dependent_rra_idx>\n",
                     rrd.rra_def[i].par[RRA_dependent_rra_idx].u_cnt);
+            cb(somestring_buf, strlen(somestring_buf), user);
             break;
         case CF_AVERAGE:
         case CF_MAXIMUM:
         case CF_MINIMUM:
         case CF_LAST:
         default:
-            fprintf(out_file, "\t\t<xff>%0.10e</xff>\n",
+            snprintf(somestring_buf, 255, "\t\t<xff>%0.10e</xff>\n",
                     rrd.rra_def[i].par[RRA_cdp_xff_val].u_val);
+            cb(somestring_buf, strlen(somestring_buf), user);
             break;
         }
-        fprintf(out_file, "\t\t</params>\n");
-        fprintf(out_file, "\t\t<cdp_prep>\n");
+
+        cb("\t\t</params>\n",
+            strlen("\t\t</params>\n"), user);
+        cb("\t\t<cdp_prep>\n",
+            strlen("\t\t<cdp_prep>\n"), user);
+
         for (ii = 0; ii < rrd.stat_head->ds_cnt; ii++) {
             unsigned long ivalue;
 
-            fprintf(out_file, "\t\t\t<ds>\n");
+            cb("\t\t\t<ds>\n",
+                strlen("\t\t\t<ds>\n"), user);
             /* support for exporting all CDP parameters */
             /* parameters common to all CFs */
             /* primary_val and secondary_val do not need to be saved between updates
              * so strictly speaking they could be omitted.
              * However, they can be useful for diagnostic purposes, so are included here. */
-            value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt
-                                 + ii].scratch[CDP_primary_val].u_val;
+            value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                scratch[CDP_primary_val].u_val;
             if (isnan(value)) {
-                fprintf(out_file,
-                        "\t\t\t<primary_value>NaN</primary_value>\n");
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<primary_value>NaN</primary_value>\n");
             } else {
-                fprintf(out_file,
-                        "\t\t\t<primary_value>%0.10e</primary_value>\n",
-                        value);
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<primary_value>%0.10e</primary_value>\n", value);
             }
-            value =
-                rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                             ii].scratch[CDP_secondary_val].u_val;
+            cb(somestring_buf, strlen(somestring_buf), user);
+
+            value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                scratch[CDP_secondary_val].u_val;
             if (isnan(value)) {
-                fprintf(out_file,
-                        "\t\t\t<secondary_value>NaN</secondary_value>\n");
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<secondary_value>NaN</secondary_value>\n");
             } else {
-                fprintf(out_file,
-                        "\t\t\t<secondary_value>%0.10e</secondary_value>\n",
-                        value);
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<secondary_value>%0.10e</secondary_value>\n", value);
             }
+            cb(somestring_buf, strlen(somestring_buf), user);
+
             switch (cf_conv(rrd.rra_def[i].cf_nam)) {
             case CF_HWPREDICT:
             case CF_MHWPREDICT:
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_intercept].u_val;
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_intercept].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file, "\t\t\t<intercept>NaN</intercept>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<intercept>NaN</intercept>\n");
                 } else {
-                    fprintf(out_file,
-                            "\t\t\t<intercept>%0.10e</intercept>\n", value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<intercept>%0.10e</intercept>\n", value);
                 }
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_last_intercept].u_val;
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_last_intercept].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file,
-                            "\t\t\t<last_intercept>NaN</last_intercept>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_intercept>NaN</last_intercept>\n");
                 } else {
-                    fprintf(out_file,
-                            "\t\t\t<last_intercept>%0.10e</last_intercept>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_intercept>%0.10e</last_intercept>\n", value);
                 }
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_slope].u_val;
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_slope].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file, "\t\t\t<slope>NaN</slope>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<slope>NaN</slope>\n");
                 } else {
-                    fprintf(out_file, "\t\t\t<slope>%0.10e</slope>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<slope>%0.10e</slope>\n", value);
                 }
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_last_slope].u_val;
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_last_slope].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file,
-                            "\t\t\t<last_slope>NaN</last_slope>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_slope>NaN</last_slope>\n");
                 } else {
-                    fprintf(out_file,
-                            "\t\t\t<last_slope>%0.10e</last_slope>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_slope>%0.10e</last_slope>\n", value);
                 }
-                ivalue =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_null_count].u_cnt;
-                fprintf(out_file, "\t\t\t<nan_count>%lu</nan_count>\n",
-                        ivalue);
-                ivalue =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_last_null_count].u_cnt;
-                fprintf(out_file,
-                        "\t\t\t<last_nan_count>%lu</last_nan_count>\n",
-                        ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                ivalue = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_null_count].u_cnt;
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<nan_count>%lu</nan_count>\n", ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                ivalue = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_last_null_count].u_cnt;
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<last_nan_count>%lu</last_nan_count>\n", ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
                 break;
             case CF_SEASONAL:
             case CF_DEVSEASONAL:
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_seasonal].u_val;
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_seasonal].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file, "\t\t\t<seasonal>NaN</seasonal>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<seasonal>NaN</seasonal>\n");
                 } else {
-                    fprintf(out_file, "\t\t\t<seasonal>%0.10e</seasonal>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<seasonal>%0.10e</seasonal>\n", value);
                 }
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_hw_last_seasonal].u_val;
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_hw_last_seasonal].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file,
-                            "\t\t\t<last_seasonal>NaN</last_seasonal>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_seasonal>NaN</last_seasonal>\n");
                 } else {
-                    fprintf(out_file,
-                            "\t\t\t<last_seasonal>%0.10e</last_seasonal>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<last_seasonal>%0.10e</last_seasonal>\n", value);
                 }
-                ivalue =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_init_seasonal].u_cnt;
-                fprintf(out_file, "\t\t\t<init_flag>%lu</init_flag>\n",
-                        ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                ivalue = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                    scratch[CDP_init_seasonal].u_cnt;
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<init_flag>%lu</init_flag>\n", ivalue);
+                cb(somestring_buf, strlen(somestring_buf), user);
                 break;
             case CF_DEVPREDICT:
                 break;
@@ -366,13 +457,16 @@
                                                                     ds_cnt +
                                                                     ii].
                                                        scratch);
-                fprintf(out_file, "\t\t\t<history> ");
+                cb("\t\t\t<history> ",
+                    strlen("\t\t\t<history> "), user);
                 for (vidx = 0;
-                     vidx < rrd.rra_def[i].par[RRA_window_len].u_cnt;
-                     ++vidx) {
-                    fprintf(out_file, "%d", violations_array[vidx]);
+                    vidx < rrd.rra_def[i].par[RRA_window_len].u_cnt;
+                    ++vidx) {
+                    snprintf(somestring_buf, 255, "%d", violations_array[vidx]);
+                    cb(somestring_buf, strlen(somestring_buf), user);
                 }
-                fprintf(out_file, " </history>\n");
+                cb(" </history>\n",
+                    strlen(" </history>\n"), user);
             }
                 break;
             case CF_AVERAGE:
@@ -380,26 +474,31 @@
             case CF_MINIMUM:
             case CF_LAST:
             default:
-                value =
-                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                 ii].scratch[CDP_val].u_val;
+                value = rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].scratch[CDP_val].u_val;
                 if (isnan(value)) {
-                    fprintf(out_file, "\t\t\t<value>NaN</value>\n");
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<value>NaN</value>\n");
                 } else {
-                    fprintf(out_file, "\t\t\t<value>%0.10e</value>\n",
-                            value);
+                    snprintf(somestring_buf, 255,
+                        "\t\t\t<value>%0.10e</value>\n", value);
                 }
-                fprintf(out_file,
-                        "\t\t\t<unknown_datapoints>%lu</unknown_datapoints>\n",
-                        rrd.cdp_prep[i * rrd.stat_head->ds_cnt +
-                                     ii].scratch[CDP_unkn_pdp_cnt].u_cnt);
+                cb(somestring_buf, strlen(somestring_buf), user);
+
+                snprintf(somestring_buf, 255,
+                    "\t\t\t<unknown_datapoints>%lu</unknown_datapoints>\n",
+                    rrd.cdp_prep[i * rrd.stat_head->ds_cnt + ii].
+                        scratch[CDP_unkn_pdp_cnt].u_cnt);
+                cb(somestring_buf, strlen(somestring_buf), user);
                 break;
             }
-            fprintf(out_file, "\t\t\t</ds>\n");
+            cb("\t\t\t</ds>\n",
+                strlen("\t\t\t</ds>\n"), user);
         }
-        fprintf(out_file, "\t\t</cdp_prep>\n");
+        cb("\t\t</cdp_prep>\n",
+            strlen("\t\t</cdp_prep>\n"), user);
 
-        fprintf(out_file, "\t\t<database>\n");
+        cb("\t\t<database>\n",
+            strlen("\t\t<database>\n"), user);
         rrd_seek(rrd_file, (rra_start + (rrd.rra_ptr[i].cur_row + 1)
                             * rrd.stat_head->ds_cnt
                             * sizeof(rrd_value_t)), SEEK_SET);
@@ -419,36 +518,75 @@
             timer++;
 #if HAVE_STRFTIME
             localtime_r(&now, &tm);
-            strftime(somestring, 200, "%Y-%m-%d %H:%M:%S %Z", &tm);
+            strftime(somestring, 255, "%Y-%m-%d %H:%M:%S %Z", &tm);
 #else
 # error "Need strftime"
 #endif
-            fprintf(out_file, "\t\t\t<!-- %s / %lld --> <row>", somestring,
-                    (long long) now);
+            snprintf(somestring_buf, 255,
+                "\t\t\t<!-- %s / %lld --> <row>",  somestring, (long long) now);
+            cb(somestring_buf, strlen(somestring_buf), user);
             for (iii = 0; iii < rrd.stat_head->ds_cnt; iii++) {
                 rrd_read(rrd_file, &my_cdp, sizeof(rrd_value_t) * 1);
                 if (isnan(my_cdp)) {
-                    fprintf(out_file, "<v>NaN</v>");
+                    snprintf(somestring_buf, 255, "<v>NaN</v>");
                 } else {
-                    fprintf(out_file, "<v>%0.10e</v>", my_cdp);
-                };
+                    snprintf(somestring_buf, 255, "<v>%0.10e</v>", my_cdp);
+                }
+                cb(somestring_buf, strlen(somestring_buf), user);
             }
-            fprintf(out_file, "</row>\n");
+            cb("</row>\n",
+                strlen("</row>\n"), user);
         }
-        fprintf(out_file, "\t\t</database>\n\t</rra>\n");
+        cb("\t\t</database>\n\t</rra>\n",
+            strlen("\t\t</database>\n\t</rra>\n"), user);
+    }
 
-    }
-    fprintf(out_file, "</rrd>\n");
+    cb("</rrd>\n",
+        strlen("</rrd>\n"), user);
+
     rrd_free(&rrd);
-    if (out_file != stdout) {
-        fclose(out_file);
-    }
+
 #ifdef HAVE_SETLOCALE
     setlocale(LC_NUMERIC, old_locale);
 #endif
+
     return rrd_close(rrd_file);
 }
 
+size_t rrd_dump_opt_cb_fileout(
+    const void *data,
+    size_t len,
+    void *user)
+{
+    return fwrite(data, 1, len, (FILE *)user);
+}
+
+int rrd_dump_opt_r(
+    const char *filename,
+    char *outname,
+    int opt_noheader)
+{
+    FILE     *out_file;
+    int       res;
+
+    out_file = NULL;
+    if (outname) {
+        if (!(out_file = fopen(outname, "w"))) {
+            return (-1);
+        }
+    } else {
+        out_file = stdout;
+    }
+
+    res = rrd_dump_cb_r(filename, opt_noheader, rrd_dump_opt_cb_fileout, (void *)out_file);
+
+    if (out_file != stdout) {
+        fclose(out_file);
+    }
+
+    return res;
+}
+
 /* backward compatibility with 1.2.x */
 int rrd_dump_r(
     const char *filename,



_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (850 bytes) Download Attachment
kevin brintnall

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
In reply to this post by Yann JOUANIN
On Sat, Aug 08, 2009 at 12:22:26PM +0200, Yann Jouanin wrote:
> It seems the behavior of RRD client when using RRDCacheD can make trouble
> when using RRDCacheD on another host than the one where using client.
>
> The translation between relative path and absolute path is done in client
> code (rrdc_flush) while it should only be done by the server.

Yann,

The client must convert to absolute path because the daemon has no idea
what the cwd is of the client.  This is true even when the client and
server run on the same machine.  i.e.:

    rrdtool update 1.rrd N:1:2
    cd elsewhere
    rrdtool update 1.rrd N:1:2

> This causes update to fail because the path is badly rewritten.
> I had to comment the following line to fix this.

You could:

 * use the same file hierarchy on client and server

 * speak to the daemon directly from your application

--
 kevin brintnall =~ /[hidden email]/

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Sebastian Harl

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
Hi Kevin, et.al.,

On Sun, Aug 09, 2009 at 03:44:41PM -0500, kevin brintnall wrote:
> On Sat, Aug 08, 2009 at 12:22:26PM +0200, Yann Jouanin wrote:
> > It seems the behavior of RRD client when using RRDCacheD can make trouble
> > when using RRDCacheD on another host than the one where using client.
> >
> > The translation between relative path and absolute path is done in client
> > code (rrdc_flush) while it should only be done by the server.

> The client must convert to absolute path because the daemon has no idea
> what the cwd is of the client.  This is true even when the client and
> server run on the same machine.  i.e.:
>
>     rrdtool update 1.rrd N:1:2
>     cd elsewhere
>     rrdtool update 1.rrd N:1:2

Hrm, I'm not sure that I agree with that. Why does the client's current
working directory matter at all to the server? Think about other ways of
remote access to files - all of them use some kind of base directory
that's used whenever relative path names are specified: scp uses $HOME,
Apache uses the document root, git-daemon supports a base path, etc. So,
I think that by following the principle of least surprise RRDcacheD
should behave like that as well. Let people think about $HOST:1.rrd
instead of 1.rrd (where $HOST might be remote or local) - we're
referring to a file on some $HOST after all …

I agree that it's not quite as obvious when using RRDcacheD locally
only (and I guess that's why it's currently implemented that way, since
that was the very first approach). Then, you'll actually see the files
in your local file system, so from a user's POV, $CWD probably does
matter. So, maybe the question is: Is RRDcacheD supposed to be a
networking daemon or rather some service that just happens to support
remote access as well because it was easy and convenient to implement
that as well? I'd probably go for the former and I'd suppose that this
is going to be the, by far, more common use case.

Then again, I guess you can find a few more pros and cons for either
way. So, maybe the actual problem is that too much magic has been put
into the way the daemon may be enabled (on the client side). By allowing
to hide the fact that, in fact, you're accessing some daemon (e.g. by
specifying an environment variable) it might not be obvious what's going
on and thus either way might be confusing for a rather large fraction of
users. OTOH, users who hide that fact have to do so explicitly, so they
should know what they are doing. So, again, I'm in favor of not letting
the client rewrite any path names.

Just my 2 ¢.

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



_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (204 bytes) Download Attachment
kevin brintnall

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
> Hrm, I'm not sure that I agree with that. Why does the client's current
> working directory matter at all to the server? Think about other ways of
> remote access to files - all of them use some kind of base directory
> that's used whenever relative path names are specified: scp uses $HOME,
> Apache uses the document root, git-daemon supports a base path, etc. So,
> I think that by following the principle of least surprise RRDcacheD
> should behave like that as well. Let people think about $HOST:1.rrd
> instead of 1.rrd (where $HOST might be remote or local) - we're
> referring to a file on some $HOST after all ???

The existing code allows the daemon to work with any existing client
work-flow without requiring them to change their code.  That is important.

--
 kevin brintnall =~ /[hidden email]/

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Benny Baumann

Re: [PATCH] Suggestion for API extension (rrd_dump)

Reply Threaded More More options
Print post
Permalink
In reply to this post by Benny Baumann
Short correction since I missed some changes in the original patch:

Regards,
BenBE.

Index: a/src/rrd_dump.c
===================================================================
--- a/src/rrd_dump.c    (old)
+++ b/src/rrd_dump.c    (new)
@@ -120,12 +120,6 @@
             strlen("<rrd>\n"), user);
     }
 
-    cb("<!-- Round Robin Database Dump -->",
-        strlen("<!-- Round Robin Database Dump -->"), user);
-
-    cb("<rrd>",
-        strlen("<rrd>"), user);
-
     if (atoi(rrd.stat_head->version) <= 3) {
         snprintf(somestring, 255, "\t<version>%s</version>\n",
RRD_VERSION3);
     } else {
@@ -207,8 +207,8 @@
             strlen("\t</ds>\n\n"), user);
     }
 
-    cb("<!-- Round Robin Archives -->\n",
-        strlen("<!-- Round Robin Archives -->\n"), user);
+    cb("\t<!-- Round Robin Archives -->\n",
+        strlen("\t<!-- Round Robin Archives -->\n"), user);
 
     rra_base = rrd_file->header_len;
     rra_next = rra_base;




_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (850 bytes) Download Attachment
Florian Forster

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
In reply to this post by Sebastian Harl
Hi,

I have talked to Sebastian in person about this, but I think it'd be
good if I repeat this here:

On Sat, Aug 08, 2009 at 12:22:26PM +0200, Yann Jouanin wrote:
> It seems the behavior of RRD client when using RRDCacheD can make
> trouble when using RRDCacheD on another host than the one where using
> client.

The intention behind RRDCacheD was to move the cache outside of the
clients and into a generic daemon. The client would communicate with the
daemon using UNIX domain sockets and in general is was just two parts of
one application glued together by some IPC. That's why the relative path
behavior basically assumes that the daemon is on the same host.

The network sockets were just added because it was only a couple of
lines extra code. They were initially not meant to be used in
production.

> The translation between relative path and absolute path is done in
> client code (rrdc_flush) while it should only be done by the server.

No. Yes. Well, this depends on whether you regard RRDCacheD as a local
acceleration cache or a remote, networked daemon.

  * If you regard RRDCacheD as a rather complex abstraction layer over
    your local RRD files, then having the client resolve relative names
    is just the thing to do. It is hard for the daemon to figure out the
    client's current working directory, as Kevin has pointed out.

      Pro: Transparent and fast update. Existing applications don't need
           to be changed.
      Con: Limitation to the same host.


  * If RRDCacheD is a normal networked daemon for you, then having a
    daemon base directory and denying all absolute paths is probably the
    best thing you can do. However, if we do that, we should definitely
    follow through: Then *all* file access has to happen via the daemon,
    not just write (update) access. This means we need to add read-
    (fetch), create-, and delete-commands as well. Sooner or later we
    also need some sort of file browsing.

      Pro: More flexibility when designing setups. Possibly better
           scalability. I have the impression a lot of people want this.
      Con: Transparency is compromised; existing applications need to
           handle this somehow. Security becomes an issue.

On Mon, Aug 10, 2009 at 10:10:38AM +0200, Sebastian Harl wrote:
> I think that by following the principle of least surprise RRDcacheD
> should behave like that as well. Let people think about $HOST:1.rrd
> instead of 1.rrd (where $HOST might be remote or local) - we're
> referring to a file on some $HOST after all …

If the daemon should work as a real network daemon, then this style of
addressing is the way to go in my opinion. IIrc this was the outcome of
our discussion about the ‘rrdc_fetch’ patch I have sent a while back (I
have some docs ready by the way, I'll send an updated patch soon).

In my ideal world, this would look somewhat like this:

  * The ‘rrd_*_r’ functions always work on local files, the ‘rrdc_*’
    always use a daemon connection. Maybe it'd be a good idea to rename
    the `rrd_*_r' functions. ‘rrdl_*’ (‘l’ as in “local”) comes to mind.

  * Some magical version of the functions will check some sort of
    environment and call the appropriate function.

  * Files are addressed using some special syntax. Using something like
      rrdcache://user@host/relative/path
    could work okay. If ‘user’ is omitted, the local user name is used.
    The paths are always relative paths so the administrator can be sure
    all the files controlled by the daemon are beneath one directory.
    File names that do not contain “://” are assumed to be local files.
    (Personally I like the “user@host:path” syntax of ‘scp’ best, but I
    fear that there could be incompatibilities.)

  * The ‘RRDCACHE_HOST’ and ‘RRDCACHE_USER’ environment variables can be
    used to avoid the special syntax. If they are set, the daemon will
    be used.

  * To authenticate, the client code looks for a file called
    ~/.rrdcache_privkey which contains a private key file (the public
    key is required on the server and should be associated with the user
    name). If found, public key authentication is attempted, possibly
    via TLS.

    If no such file exists, the code will check for a password in
    ~/.rrdcache_password and, if found, will use it for password
    authentication.

    If neither file exists and the process has a controlling terminal,
    then it will try to read a password from /dev/console.

    If all else fails, try authenticating with an empty password.

> So, maybe the actual problem is that too much magic has been put into
> the way the daemon may be enabled (on the client side).

I hope the environment variables don't add too much confusion. Other
than that, I think the above approach is pretty straight forward and
easy to understand.

Regards,
-octo
--
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (196 bytes) Download Attachment
oetiker

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
Hi Florian,

Yesterday Florian Forster wrote:

> In my ideal world, this would look somewhat like this:
>
>   * The ?rrd_*_r? functions always work on local files, the ?rrdc_*?
>     always use a daemon connection. Maybe it'd be a good idea to rename
>     the `rrd_*_r' functions. ?rrdl_*? (?l? as in ?local?) comes to mind.
>
>   * Some magical version of the functions will check some sort of
>     environment and call the appropriate function.

this is to maintain the present interface ?

>   * Files are addressed using some special syntax. Using something like
>       rrdcache://user@host/relative/path
>     could work okay. If ?user? is omitted, the local user name is used.
>     The paths are always relative paths so the administrator can be sure
>     all the files controlled by the daemon are beneath one directory.
>     File names that do not contain ?://? are assumed to be local files.
>     (Personally I like the ?user@host:path? syntax of ?scp? best, but I
>     fear that there could be incompatibilities.)
>
>   * The ?RRDCACHE_HOST? and ?RRDCACHE_USER? environment variables can be
>     used to avoid the special syntax. If they are set, the daemon will
>     be used.
>
>   * To authenticate, the client code looks for a file called
>     ~/.rrdcache_privkey which contains a private key file (the public
>     key is required on the server and should be associated with the user
>     name). If found, public key authentication is attempted, possibly
>     via TLS.
>
>     If no such file exists, the code will check for a password in
>     ~/.rrdcache_password and, if found, will use it for password
>     authentication.
>
>     If neither file exists and the process has a controlling terminal,
>     then it will try to read a password from /dev/console.
>
>     If all else fails, try authenticating with an empty password.

this does sound fine to me ... I will make a note of it on the wiki

I guess its time to finally push out 1.4 so that the code can move
again ...

cheers
tobi

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Florian Forster

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
Hi Tobi,

On Tue, Aug 11, 2009 at 07:48:30AM +0200, Tobias Oetiker wrote:
> >   * Some magical version of the functions will check some sort of
> >     environment and call the appropriate function.
>
> this is to maintain the present interface ?

well, I would much rather see the end of that argv/argc style interface
and have all functions use an interface in the style of the `*_r'
functions. But as I said: That's my ideal dreamworld.

Maybe a possible transition would be to break ABI compatibility but
provide an optional legacy API. I. e. rename `rrd_update' and friends to
`rrd_update_legacy' (this brakes ABI compatibility). Then provide
something like this:

  /* declaration of rrd_update_local and rrd_update_remote */
  #if RRD_LEGACY_INTERFACE
  /* declaration of rrd_update_legacy */
  # define rrd_update   rrd_update_legacy
  # define rrd_update_r rrd_update_local
  # define rrdc_update  rrd_update_remote
  ...
  #else
  /* declaration of rrd_update */
  #endif

> I guess its time to finally push out 1.4 so that the code can move
> again ...

I'd be glad if you could consider my per-socket authorization patch for
the 1.4 release. I think it's a big improvement over the ``restricted
sockets'' (the `-L' command line option). I'll send an updates patch
with docs in a minute..

Regards,
-octo
--
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (196 bytes) Download Attachment
oetiker

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
Today Florian Forster wrote:

> Hi Tobi,
>
> On Tue, Aug 11, 2009 at 07:48:30AM +0200, Tobias Oetiker wrote:
> > >   * Some magical version of the functions will check some sort of
> > >     environment and call the appropriate function.
> >
> > this is to maintain the present interface ?
>
> well, I would much rather see the end of that argv/argc style interface
> and have all functions use an interface in the style of the `*_r'
> functions. But as I said: That's my ideal dreamworld.
>
> Maybe a possible transition would be to break ABI compatibility but
> provide an optional legacy API. I. e. rename `rrd_update' and friends to
> `rrd_update_legacy' (this brakes ABI compatibility). Then provide
> something like this:
>
>   /* declaration of rrd_update_local and rrd_update_remote */
>   #if RRD_LEGACY_INTERFACE
>   /* declaration of rrd_update_legacy */
>   # define rrd_update   rrd_update_legacy
>   # define rrd_update_r rrd_update_local
>   # define rrdc_update  rrd_update_remote
>   ...
>   #else
>   /* declaration of rrd_update */
>   #endif

that does sound reasonable ... I think it is important to allow for
a backward compatible interface ...

> > I guess its time to finally push out 1.4 so that the code can move
> > again ...
>
> I'd be glad if you could consider my per-socket authorization patch for
> the 1.4 release. I think it's a big improvement over the ``restricted
> sockets'' (the `-L' command line option). I'll send an updates patch
> with docs in a minute..

to me this patch looks good ... as you know the whole inet socket
interface without authentication is a thorn in my side. your patch
makes this a bit better ...

I'll wait to hear what kevin has to say on the patch.

cheers
tobi
>
> Regards,
> -octo
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Sebastian Harl

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
In reply to this post by Florian Forster
Hi,

On Mon, Aug 10, 2009 at 09:51:26PM +0200, Florian Forster wrote:
> On Sat, Aug 08, 2009 at 12:22:26PM +0200, Yann Jouanin wrote:
> > The translation between relative path and absolute path is done in
> > client code (rrdc_flush) while it should only be done by the server.
>
> No. Yes. Well, this depends on whether you regard RRDCacheD as a local
> acceleration cache or a remote, networked daemon.
[…]

Thanks for being a bit more verbose about that than I was. I fully agree
with this differentiation.

> On Mon, Aug 10, 2009 at 10:10:38AM +0200, Sebastian Harl wrote:
> > I think that by following the principle of least surprise RRDcacheD
> > should behave like that as well. Let people think about $HOST:1.rrd
> > instead of 1.rrd (where $HOST might be remote or local) - we're
> > referring to a file on some $HOST after all …
>
> If the daemon should work as a real network daemon, then this style of
> addressing is the way to go in my opinion.

Imho, if we're going to decide that this is _not_ what we want in the
long run, we should seriously think about removing networking support
again (which would have to happen now, before releasing 1.4). Imho,
having something that exist just because it was easy to implement but is
not really supported (as in: having some somewhat "half-baked" solution)
by the developers causes more pain than good.

Anyway, I don't think that this is what you want, so lets try to make
this a real network daemon :-)

> In my ideal world, this would look somewhat like this:
>
>   * The ‘rrd_*_r’ functions always work on local files, the ‘rrdc_*’
>     always use a daemon connection.

Ack! Let's not put too much magic into the core.

>   * Some magical version of the functions will check some sort of
>     environment and call the appropriate function.

That would imho be fine though as long as a (library) user still has
access to non-magical functions as well to be able to _explicitly_ state
what's supposed to be going on.

>   * Files are addressed using some special syntax. Using something like
>       rrdcache://user@host/relative/path
>     could work okay. If ‘user’ is omitted, the local user name is used.
>     The paths are always relative paths so the administrator can be sure
>     all the files controlled by the daemon are beneath one directory.
>     File names that do not contain “://” are assumed to be local files.

Sounds good to me.

>     (Personally I like the “user@host:path” syntax of ‘scp’ best, but I
>     fear that there could be incompatibilities.)

Hrm … scp needs that to be able to tell apart absolute path names from
relative path names. Since we don't need / want that, I'd actually
prefer the solution suggested above.

>   * The ‘RRDCACHE_HOST’ and ‘RRDCACHE_USER’ environment variables can be
>     used to avoid the special syntax. If they are set, the daemon will
>     be used.

So, if those variables are defined, a user could simply use some
relative path name ("some/path/name.rrd") to access the daemon and that
path would be sent to the daemon without any modifications on the client
side? Imho, that would be the only sensible approach - handling some
special cases differently imho introduces too much magic that would
definitely cause a lot more confusion.

>   * To authenticate, the client code looks for a file called
>     ~/.rrdcache_privkey which contains a private key file (the public
>     key is required on the server and should be associated with the user
>     name). If found, public key authentication is attempted, possibly
>     via TLS.
>
>     If no such file exists, the code will check for a password in
>     ~/.rrdcache_password and, if found, will use it for password
>     authentication.
>
>     If neither file exists and the process has a controlling terminal,
>     then it will try to read a password from /dev/console.
>
>     If all else fails, try authenticating with an empty password.
Sounds good imho.

> > So, maybe the actual problem is that too much magic has been put into
> > the way the daemon may be enabled (on the client side).
>
> I hope the environment variables don't add too much confusion.

Well, that has the most potential for causing confusion but, then again,
the user should know what she's doing when using such features.

> Other than that, I think the above approach is pretty straight forward
> and easy to understand.

Full ack!

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



_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (204 bytes) Download Attachment
Sebastian Harl

Re: RRDCacheD - Client rewriting path

Reply Threaded More More options
Print post
Permalink
In reply to this post by oetiker
Hi Tobi,

On Tue, Aug 11, 2009 at 07:48:30AM +0200, Tobias Oetiker wrote:
> I guess its time to finally push out 1.4 so that the code can move
> again ...

As much as I'd hate to further delay the 1.4 release, I think we should
come to a conclusion about further directions first. Else, we might end
up with some functionality (I'm e.g. thinking about the client side path
"translation") that's going to be hard to support in the future. I know
how much you hate to break backward-compatibility, so let's try to avoid
some trouble trying to keep that up.

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



_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

signature.asc (204 bytes) Download Attachment
1 2 3 4