From: oetiker <oetiker@a5681a0c-68f1-0310-ab6d-d61299d08faa>
Date: Wed, 30 May 2007 12:39:30 +0000 (+0000)
Subject: From Bernhard Fischer
X-Git-Url: https://git.verplant.org/?a=commitdiff_plain;h=7f30ee2ae089520606cbb7e38fab9981618771f5;p=rrdtool.git

From Bernhard Fischer

- rrd_close(): call close on the file and use rrd_close consistently
- clean up some error paths. The fadvise error path is leaking memory (see XXX in these spots).



git-svn-id: svn://svn.oetiker.ch/rrdtool/trunk/program@1092 a5681a0c-68f1-0310-ab6d-d61299d08faa
---

diff --git a/src/rrd_create.c b/src/rrd_create.c
index d36cf3c..35a06b1 100644
--- a/src/rrd_create.c
+++ b/src/rrd_create.c
@@ -89,7 +89,7 @@ int rrd_create(
         }
     }
     if (optind == argc) {
-        rrd_set_error("what is the name of the rrd file you want to create?");
+        rrd_set_error("need name of an rrd file to create");
         return -1;
     }
     rc = rrd_create_r(argv[optind],
diff --git a/src/rrd_dump.c b/src/rrd_dump.c
index 0b93103..316814d 100644
--- a/src/rrd_dump.c
+++ b/src/rrd_dump.c
@@ -421,9 +421,8 @@ int rrd_dump_r(
     }
     fprintf(out_file, "</rrd>\n");
     rrd_free(&rrd);
-    close(rrd_file->fd);
     if (out_file != stdout) {
         fclose(out_file);
     }
-    return (0);
+    return rrd_close(rrd_file);
 }
diff --git a/src/rrd_fetch.c b/src/rrd_fetch.c
index 87c3cd6..618f3d3 100644
--- a/src/rrd_fetch.c
+++ b/src/rrd_fetch.c
@@ -156,7 +156,7 @@ int rrd_fetch(
     cf = argv[optind + 1];
 
     if (rrd_fetch_r(argv[optind], cf, start, end, step, ds_cnt, ds_namv, data)
-        == -1)
+        != 0)
         return (-1);
     return (0);
 }
@@ -218,25 +218,20 @@ int rrd_fetch_fn(
 
     rrd_file = rrd_open(filename, &rrd, RRD_READONLY);
     if (rrd_file == NULL)
-        return (-1);
+        goto err_free;
 
     /* when was the really last update of this file ? */
 
     if (((*ds_namv) =
          (char **) malloc(rrd.stat_head->ds_cnt * sizeof(char *))) == NULL) {
         rrd_set_error("malloc fetch ds_namv array");
-        rrd_free(&rrd);
-        close(rrd_file->fd);
-        return (-1);
+        goto err_close;
     }
 
     for (i = 0; (unsigned long) i < rrd.stat_head->ds_cnt; i++) {
         if ((((*ds_namv)[i]) = malloc(sizeof(char) * DS_NAM_SIZE)) == NULL) {
             rrd_set_error("malloc fetch ds_namv entry");
-            rrd_free(&rrd);
-            free(*ds_namv);
-            close(rrd_file->fd);
-            return (-1);
+            goto err_free_ds_namv;
         }
         strncpy((*ds_namv)[i], rrd.ds_def[i].ds_nam, DS_NAM_SIZE - 1);
         (*ds_namv)[i][DS_NAM_SIZE - 1] = '\0';
@@ -274,9 +269,7 @@ int rrd_fetch_fn(
                     best_full_rra = i;
 #ifdef DEBUG
                     fprintf(stderr, "best full match so far\n");
-#endif
                 } else {
-#ifdef DEBUG
                     fprintf(stderr, "full match, not best\n");
 #endif
                 }
@@ -316,9 +309,7 @@ int rrd_fetch_fn(
     else {
         rrd_set_error
             ("the RRD does not contain an RRA matching the chosen CF");
-        rrd_free(&rrd);
-        close(rrd_file->fd);
-        return (-1);
+        goto err_free_all_ds_namv;
     }
 
     /* set the wish parameters to their real values */
@@ -342,12 +333,7 @@ int rrd_fetch_fn(
     *ds_cnt = rrd.stat_head->ds_cnt;
     if (((*data) = malloc(*ds_cnt * rows * sizeof(rrd_value_t))) == NULL) {
         rrd_set_error("malloc fetch data area");
-        for (i = 0; (unsigned long) i < *ds_cnt; i++)
-            free((*ds_namv)[i]);
-        free(*ds_namv);
-        rrd_free(&rrd);
-        close(rrd_file->fd);
-        return (-1);
+        goto err_free_all_ds_namv;
     }
 
     data_ptr = (*data);
@@ -382,15 +368,7 @@ int rrd_fetch_fn(
                                         * sizeof(rrd_value_t))),
                  SEEK_SET) != 0) {
         rrd_set_error("seek error in RRA");
-        for (i = 0; (unsigned) i < *ds_cnt; i++)
-            free((*ds_namv)[i]);
-        free(*ds_namv);
-        rrd_free(&rrd);
-        free(*data);
-        *data = NULL;
-        close(rrd_file->fd);
-        return (-1);
-
+        goto err_free_data;
     }
 #ifdef DEBUG
     fprintf(stderr, "First Seek: rra_base %lu rra_pointer %lu\n",
@@ -432,14 +410,7 @@ int rrd_fetch_fn(
                                         * sizeof(rrd_value_t)),
                              SEEK_SET) != 0) {
                     rrd_set_error("wrap seek in RRA did fail");
-                    for (ii = 0; (unsigned) ii < *ds_cnt; ii++)
-                        free((*ds_namv)[ii]);
-                    free(*ds_namv);
-                    rrd_free(&rrd);
-                    free(*data);
-                    *data = NULL;
-                    close(rrd_file->fd);
-                    return (-1);
+                    goto err_free_data;
                 }
 #ifdef DEBUG
                 fprintf(stderr, "wrap seek ...\n");
@@ -449,14 +420,7 @@ int rrd_fetch_fn(
             if (rrd_read(rrd_file, data_ptr, sizeof(rrd_value_t) * (*ds_cnt))
                 != (ssize_t) (sizeof(rrd_value_t) * (*ds_cnt))) {
                 rrd_set_error("fetching cdp from rra");
-                for (ii = 0; (unsigned) ii < *ds_cnt; ii++)
-                    free((*ds_namv)[ii]);
-                free(*ds_namv);
-                rrd_free(&rrd);
-                free(*data);
-                *data = NULL;
-                close(rrd_file->fd);
-                return (-1);
+                goto err_free_data;
             }
 #ifdef HAVE_POSIX_FADVISE
             /* don't pollute the buffer cache with data read from the file. We do this while reading to 
@@ -466,8 +430,7 @@ int rrd_fetch_fn(
                               POSIX_FADV_DONTNEED)) {
                 rrd_set_error("setting POSIX_FADV_DONTNEED on '%s': %s",
                               filename, rrd_strerror(errno));
-                close(rrd_file->fd);
-                return (-1);
+                goto err_close;/*XXX: should use err_free_all_ds_namv */
             }
 #endif
 
@@ -484,7 +447,6 @@ int rrd_fetch_fn(
 #endif
 
     }
-    rrd_free(&rrd);
 #ifdef HAVE_POSIX_FADVISE
     /* and just to be sure we drop everything except the header at the end */
     if (0 !=
@@ -492,10 +454,22 @@ int rrd_fetch_fn(
                       POSIX_FADV_DONTNEED)) {
         rrd_set_error("setting POSIX_FADV_DONTNEED on '%s': %s", filename,
                       rrd_strerror(errno));
-        close(rrd_file->fd);
-        return (-1);
+        goto err_free; /*XXX: should use err_free_all_ds_namv */
     }
 #endif
-    close(rrd_file->fd);
+    rrd_close(rrd_file);
     return (0);
+err_free_data:
+    free(*data);
+    *data = NULL;
+err_free_all_ds_namv:
+    for (i = 0; (unsigned long)i < rrd.stat_head->ds_cnt; ++i)
+        free((*ds_namv)[i]);
+err_free_ds_namv:
+    free(*ds_namv);
+err_close:
+    rrd_close(rrd_file);
+err_free:
+    rrd_free(&rrd);
+    return (-1);
 }
diff --git a/src/rrd_first.c b/src/rrd_first.c
index 0bd3f45..bb86947 100644
--- a/src/rrd_first.c
+++ b/src/rrd_first.c
@@ -61,21 +61,18 @@ time_t rrd_first_r(
     const int rraindex)
 {
     off_t     rra_start, timer;
-    time_t    then;
+    time_t    then = -1;
     rrd_t     rrd;
     rrd_file_t *rrd_file;
 
     rrd_file = rrd_open(filename, &rrd, RRD_READONLY);
     if (rrd_file == NULL) {
-        rrd_set_error("could not open RRD");
-        return (-1);
+        goto err_free;
     }
 
     if ((rraindex < 0) || (rraindex >= (int) rrd.stat_head->rra_cnt)) {
         rrd_set_error("invalid rraindex number");
-        rrd_free(&rrd);
-        close(rrd_file->fd);
-        return (-1);
+        goto err_close;
     }
 
     rra_start = rrd_file->header_len;
@@ -91,9 +88,9 @@ time_t rrd_first_r(
             rrd.live_head->last_up %
             (rrd.rra_def[rraindex].pdp_cnt * rrd.stat_head->pdp_step)) +
         (timer * rrd.rra_def[rraindex].pdp_cnt * rrd.stat_head->pdp_step);
-
-    rrd_free(&rrd);
-    close(rrd_file->fd);
+err_close:
     rrd_close(rrd_file);
+err_free:
+    rrd_free(&rrd);
     return (then);
 }
diff --git a/src/rrd_info.c b/src/rrd_info.c
index d0d9db5..e5d195f 100644
--- a/src/rrd_info.c
+++ b/src/rrd_info.c
@@ -99,17 +99,15 @@ info_t   *rrd_info_r(
 {
     unsigned int i, ii = 0;
     rrd_t     rrd;
-    info_t   *data, *cd;
+    info_t   *data = NULL, *cd;
     infoval   info;
     rrd_file_t *rrd_file;
     enum cf_en current_cf;
     enum dst_en current_ds;
 
     rrd_file = rrd_open(filename, &rrd, RRD_READONLY);
-    if (rrd_file == NULL) {
-        return (NULL);
-    }
-    close(rrd_file->fd);
+    if (rrd_file == NULL)
+        goto err_free;
 
     info.u_str = filename;
     cd = info_push(NULL, sprintf_alloc("filename"), RD_I_STR, info);
@@ -306,8 +304,9 @@ info_t   *rrd_info_r(
             }
         }
     }
-    rrd_free(&rrd);
+
     rrd_close(rrd_file);
+err_free:
+    rrd_free(&rrd);
     return (data);
-
 }
diff --git a/src/rrd_last.c b/src/rrd_last.c
index 42a8075..90da610 100644
--- a/src/rrd_last.c
+++ b/src/rrd_last.c
@@ -24,18 +24,16 @@ time_t rrd_last(
 time_t rrd_last_r(
     const char *filename)
 {
-    time_t    lastup;
+    time_t    lastup = -1;
     rrd_file_t *rrd_file;
 
     rrd_t     rrd;
 
     rrd_file = rrd_open(filename, &rrd, RRD_READONLY);
-    if (rrd_file == NULL)
-        return (-1);
-
-    lastup = rrd.live_head->last_up;
+    if (rrd_file != NULL) {
+        lastup = rrd.live_head->last_up;
+        rrd_close(rrd_file);
+    }
     rrd_free(&rrd);
-    close(rrd_file->fd);
-    rrd_close(rrd_file);
     return (lastup);
 }
diff --git a/src/rrd_lastupdate.c b/src/rrd_lastupdate.c
index 79274b5..046a173 100644
--- a/src/rrd_lastupdate.c
+++ b/src/rrd_lastupdate.c
@@ -23,29 +23,26 @@ int rrd_lastupdate(
 
     if (argc < 2) {
         rrd_set_error("please specify an rrd");
-        return -1;
+        goto err_out;
     }
     filename = argv[1];
 
     rrd_file = rrd_open(filename, &rrd, RRD_READONLY);
     if (rrd_file == NULL)
-        return (-1);
+        goto err_free;
 
     *last_update = rrd.live_head->last_up;
     *ds_cnt = rrd.stat_head->ds_cnt;
     if (((*ds_namv) =
          (char **) malloc(rrd.stat_head->ds_cnt * sizeof(char *))) == NULL) {
         rrd_set_error("malloc fetch ds_namv array");
-        rrd_free(&rrd);
-        return (-1);
+        goto err_close;
     }
 
     if (((*last_ds) =
          (char **) malloc(rrd.stat_head->ds_cnt * sizeof(char *))) == NULL) {
         rrd_set_error("malloc fetch last_ds array");
-        rrd_free(&rrd);
-        free(*ds_namv);
-        return (-1);
+        goto err_free_ds_namv;
     }
 
     for (i = 0; i < rrd.stat_head->ds_cnt; i++) {
@@ -54,7 +51,15 @@ int rrd_lastupdate(
     }
 
     rrd_free(&rrd);
-    close(rrd_file->fd);
     rrd_close(rrd_file);
     return (0);
+
+err_free_ds_namv:
+    free(*ds_namv);
+err_close:
+    rrd_close(rrd_file);
+err_free:
+    rrd_free(&rrd);
+err_out:
+    return (-1);
 }
diff --git a/src/rrd_open.c b/src/rrd_open.c
index c986f75..e182091 100644
--- a/src/rrd_open.c
+++ b/src/rrd_open.c
@@ -68,9 +68,9 @@
 
 /* DEBUG 2 prints information obtained via mincore(2) */
 // #define DEBUG 2
-/* do not calculate exact madvise hints but assume 2 pages for headers and
+/* do not calculate exact madvise hints but assume 1 page for headers and
  * set DONTNEED for the rest, which is assumed to be data */
-//#define TWO_PAGES 1
+//#define ONE_PAGE 1
 /* Avoid calling madvise on areas that were already hinted. May be benefical if
  * your syscalls are very slow */
 //#define CHECK_MADVISE_OVERLAPS 1
@@ -117,8 +117,11 @@ _madvise_vec_t _madv_vec = { NULL, 0 };
     madvise((_start), (_off), (_hint))
 #endif
 
-/* open a database file, return its header and an open filehandle */
-/* positioned to the first cdp in the first rra */
+/* Open a database file, return its header and an open filehandle,
+ * positioned to the first cdp in the first rra.
+ * In the error path of rrd_open, only rrd_free(&rrd) has to be called
+ * before returning an error. Do not call rrd_close upon failure of rrd_open.
+ */
 
 rrd_file_t *rrd_open(
     const char *const file_name,
@@ -136,14 +139,16 @@ rrd_file_t *rrd_open(
 #endif
     off_t     offset = 0;
     struct stat statb;
-    rrd_file_t *rrd_file = malloc(sizeof(rrd_file_t));
+    rrd_file_t *rrd_file = NULL;
 
+    rrd_init(rrd);
+    rrd_file = malloc(sizeof(rrd_file_t));
     if (rrd_file == NULL) {
         rrd_set_error("allocating rrd_file descriptor for '%s'", file_name);
         return NULL;
     }
     memset(rrd_file, 0, sizeof(rrd_file_t));
-    rrd_init(rrd);
+
 #ifdef DEBUG
     if ((rdwr & (RRD_READONLY | RRD_READWRITE)) ==
         (RRD_READONLY | RRD_READWRITE)) {
@@ -242,19 +247,19 @@ rrd_file_t *rrd_open(
         goto out_done;
     }
     /* We do not need to read anything in for the moment */
-#ifndef TWO_PAGES
+#ifndef ONE_PAGE
     _madvise(data, rrd_file->file_len, MADV_DONTNEED);
 //    _madvise(data, rrd_file->file_len, MADV_RANDOM);
 #else
 /* alternatively: keep 2 pages worth of data, likely headers,
  * don't need the rest.  */
-    _madvise(data, _page_size * 2, MADV_WILLNEED | MADV_SEQUENTIAL);
-    _madvise(data + _page_size * 2, (rrd_file->file_len >= _page_size * 2)
-             ? rrd_file->file_len - _page_size * 2 : 0, MADV_DONTNEED);
+    _madvise(data, _page_size, MADV_WILLNEED | MADV_SEQUENTIAL);
+    _madvise(data + _page_size, (rrd_file->file_len >= _page_size)
+             ? rrd_file->file_len - _page_size : 0, MADV_DONTNEED);
 #endif
 #endif
 
-#if defined USE_MADVISE && !defined TWO_PAGES
+#if defined USE_MADVISE && !defined ONE_PAGE
     /* the stat_head will be needed soonish, so hint accordingly */
 // too finegrained to calc the individual sizes, just keep 2 pages worth of hdr
     _madvise(data + PAGE_ALIGN_DOWN(offset), PAGE_ALIGN(sizeof(stat_head_t)),
@@ -283,7 +288,7 @@ rrd_file_t *rrd_open(
                       rrd->stat_head->version);
         goto out_nullify_head;
     }
-#if defined USE_MADVISE && !defined TWO_PAGES
+#if defined USE_MADVISE && !defined ONE_PAGE
     /* the ds_def will be needed soonish, so hint accordingly */
     _madvise(data + PAGE_ALIGN_DOWN(offset),
              PAGE_ALIGN(sizeof(ds_def_t) * rrd->stat_head->ds_cnt),
@@ -292,7 +297,7 @@ rrd_file_t *rrd_open(
     __rrd_read(rrd->ds_def, ds_def_t,
                rrd->stat_head->ds_cnt);
 
-#if defined USE_MADVISE && !defined TWO_PAGES
+#if defined USE_MADVISE && !defined ONE_PAGE
     /* the rra_def will be needed soonish, so hint accordingly */
     _madvise(data + PAGE_ALIGN_DOWN(offset),
              PAGE_ALIGN(sizeof(rra_def_t) * rrd->stat_head->rra_cnt),
@@ -316,7 +321,7 @@ rrd_file_t *rrd_open(
 #endif
         rrd->live_head->last_up_usec = 0;
     } else {
-#if defined USE_MADVISE && !defined TWO_PAGES
+#if defined USE_MADVISE && !defined ONE_PAGE
         /* the live_head will be needed soonish, so hint accordingly */
         _madvise(data + PAGE_ALIGN_DOWN(offset),
                  PAGE_ALIGN(sizeof(live_head_t)), MADV_WILLNEED);
@@ -358,7 +363,7 @@ int rrd_close(
 {
     int       ret;
 
-#if defined HAVE_MMAP
+#if defined HAVE_MMAP || defined DEBUG
     ssize_t   _page_size = sysconf(_SC_PAGESIZE);
 #endif
 #if defined DEBUG && DEBUG > 1
@@ -366,10 +371,8 @@ int rrd_close(
     off_t     off;
     unsigned char *vec;
 
-    off =
-        rrd_file->file_len +
-        ((rrd_file->file_len + sysconf(_SC_PAGESIZE) -
-          1) / sysconf(_SC_PAGESIZE));
+    off = rrd_file->file_len +
+        ((rrd_file->file_len + _page_size - 1) / _page_size);
     vec = malloc(off);
     if (vec != NULL) {
         memset(vec, 0, off);
@@ -397,9 +400,8 @@ int rrd_close(
 #endif                          /* DEBUG */
 
 #ifdef USE_MADVISE
-#ifdef TWO_PAGES
-//XXX: ?
-    /* Keep 2 pages worth of headers around, round up to next page boundary.  */
+#ifdef ONE_PAGE
+    /* Keep headers around, round up to next page boundary.  */
     ret =
         PAGE_ALIGN(rrd_file->header_len % _page_size + rrd_file->header_len);
     if (rrd_file->file_len > ret)
@@ -416,12 +418,10 @@ int rrd_close(
     ret = munmap(rrd_file->file_start, rrd_file->file_len);
     if (ret != 0)
         rrd_set_error("munmap rrd_file: %s", rrd_strerror(errno));
-#else
-    ret = 0;
 #endif
-//    ret = close(rrd_file->fd);
-//    if (ret != 0)
-//        rrd_set_error("closing file: %s", rrd_strerror(errno));
+    ret = close(rrd_file->fd);
+    if (ret != 0)
+        rrd_set_error("closing file: %s", rrd_strerror(errno));
     free(rrd_file);
     rrd_file = NULL;
     return ret;
@@ -534,10 +534,13 @@ void rrd_init(
 
 /* free RRD header data.  */
 
+#ifdef HAVE_MMAP
+inline void rrd_free(
+    rrd_t UNUSED(*rrd)) {}
+#else
 void rrd_free(
-    rrd_t UNUSED(*rrd))
+    rrd_t *rrd)
 {
-#ifndef HAVE_MMAP
     if (atoi(rrd->stat_head->version) < 3)
         free(rrd->live_head);
     free(rrd->stat_head);
@@ -547,8 +550,8 @@ void rrd_free(
     free(rrd->pdp_prep);
     free(rrd->cdp_prep);
     free(rrd->rrd_value);
-#endif
 }
+#endif
 
 
 /* routine used by external libraries to free memory allocated by
diff --git a/src/rrd_resize.c b/src/rrd_resize.c
index 11efcda..09927df 100644
--- a/src/rrd_resize.c
+++ b/src/rrd_resize.c
@@ -57,20 +57,20 @@ int rrd_resize(
 
     rrd_file = rrd_open(infilename, &rrdold, RRD_READWRITE | RRD_COPY);
     if (rrd_file == NULL) {
-        rrd_set_error("could not open RRD");
+        rrd_free(&rrdold);
         return (-1);
     }
     if (LockRRD(rrd_file->fd) != 0) {
         rrd_set_error("could not lock original RRD");
         rrd_free(&rrdold);
-        close(rrd_file->fd);
+        rrd_close(rrd_file);
         return (-1);
     }
 
     if (target_rra >= rrdold.stat_head->rra_cnt) {
         rrd_set_error("no such RRA in this RRD");
         rrd_free(&rrdold);
-        close(rrd_file->fd);
+        rrd_close(rrd_file);
         return (-1);
     }
 
@@ -78,7 +78,7 @@ int rrd_resize(
         if ((long) rrdold.rra_def[target_rra].row_cnt <= -modify) {
             rrd_set_error("This RRA is not that big");
             rrd_free(&rrdold);
-            close(rrd_file->fd);
+            rrd_close(rrd_file);
             return (-1);
         }
 
@@ -86,13 +86,14 @@ int rrd_resize(
     if (rrd_out_file == NULL) {
         rrd_set_error("Can't create '%s': %s", outfilename,
                       rrd_strerror(errno));
+	rrd_free(&rrdnew);
         return (-1);
     }
     if (LockRRD(rrd_out_file->fd) != 0) {
         rrd_set_error("could not lock new RRD");
         rrd_free(&rrdold);
-        close(rrd_file->fd);
-        close(rrd_out_file->fd);
+        rrd_close(rrd_file);
+        rrd_close(rrd_out_file);
         return (-1);
     }
 /*XXX: do one write for those parts of header that are unchanged */
@@ -231,11 +232,9 @@ int rrd_resize(
               sizeof(rra_ptr_t) * rrdnew.stat_head->rra_cnt);
 
     rrd_free(&rrdold);
-    close(rrd_file->fd);
     rrd_close(rrd_file);
 
     rrd_free(&rrdnew);
-    close(rrd_out_file->fd);
     rrd_close(rrd_out_file);
 
     return (0);
diff --git a/src/rrd_tune.c b/src/rrd_tune.c
index 680e558..91f6409 100644
--- a/src/rrd_tune.c
+++ b/src/rrd_tune.c
@@ -79,6 +79,7 @@ int rrd_tune(
 
     rrd_file = rrd_open(argv[1], &rrd, RRD_READWRITE);
     if (rrd_file == NULL) {
+        rrd_free(&rrd);
         return -1;
     }
 
@@ -117,12 +118,12 @@ int rrd_tune(
                         &heartbeat)) != 2) {
                 rrd_set_error("invalid arguments for heartbeat");
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             if ((ds = ds_match(&rrd, ds_nam)) == -1) {
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             rrd.ds_def[ds].par[DS_mrhb_cnt].u_cnt = heartbeat;
@@ -133,12 +134,12 @@ int rrd_tune(
                  sscanf(optarg, DS_NAM_FMT ":%lf", ds_nam, &min)) < 1) {
                 rrd_set_error("invalid arguments for minimum ds value");
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             if ((ds = ds_match(&rrd, ds_nam)) == -1) {
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
 
@@ -152,12 +153,12 @@ int rrd_tune(
                  sscanf(optarg, DS_NAM_FMT ":%lf", ds_nam, &max)) < 1) {
                 rrd_set_error("invalid arguments for maximum ds value");
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             if ((ds = ds_match(&rrd, ds_nam)) == -1) {
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             if (matches == 1)
@@ -170,17 +171,17 @@ int rrd_tune(
                  sscanf(optarg, DS_NAM_FMT ":" DST_FMT, ds_nam, dst)) != 2) {
                 rrd_set_error("invalid arguments for data source type");
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             if ((ds = ds_match(&rrd, ds_nam)) == -1) {
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             if ((int) dst_conv(dst) == -1) {
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             strncpy(rrd.ds_def[ds].dst, dst, DST_SIZE - 1);
@@ -199,12 +200,12 @@ int rrd_tune(
                         ds_new)) != 2) {
                 rrd_set_error("invalid arguments for data source type");
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             if ((ds = ds_match(&rrd, ds_nam)) == -1) {
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             strncpy(rrd.ds_def[ds].ds_nam, ds_new, DS_NAM_SIZE - 1);
@@ -262,19 +263,19 @@ int rrd_tune(
             if (sscanf(optarg, DS_NAM_FMT, ds_nam) != 1) {
                 rrd_set_error("invalid argument for aberrant-reset");
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             if ((ds = ds_match(&rrd, ds_nam)) == -1) {
                 /* ds_match handles it own errors */
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             reset_aberrant_coefficients(&rrd, rrd_file, (unsigned long) ds);
             if (rrd_test_error()) {
                 rrd_free(&rrd);
-                close(rrd_file->fd);
+                rrd_close(rrd_file);
                 return -1;
             }
             break;
@@ -284,7 +285,7 @@ int rrd_tune(
             else
                 rrd_set_error("unknown option '%s'", argv[optind - 1]);
             rrd_free(&rrd);
-            close(rrd_file->fd);
+            rrd_close(rrd_file);
             return -1;
         }
     }
@@ -318,7 +319,7 @@ int rrd_tune(
                 free(buffer);
             }
     }
-    close(rrd_file->fd);
+    rrd_close(rrd_file);
     rrd_free(&rrd);
     return 0;
 }
diff --git a/src/rrd_update.c b/src/rrd_update.c
index 6ef5d3f..18db8e7 100644
--- a/src/rrd_update.c
+++ b/src/rrd_update.c
@@ -323,7 +323,7 @@ int _rrd_update(
 
     rrd_file = rrd_open(filename, &rrd, RRD_READWRITE);
     if (rrd_file == NULL) {
-        goto err_out;
+        goto err_free;
     }
 
     /* initialize time */
@@ -446,7 +446,7 @@ int _rrd_update(
         free(pdp_temp);
         free(tmpl_idx);
         rrd_free(&rrd);
-        close(rrd_file->fd);
+        rrd_close(rrd_file);
         return (-1);
     }
 #ifdef USE_MADVISE
@@ -1531,7 +1531,6 @@ int _rrd_update(
     }
 
     rrd_free(&rrd);
-    close(rrd_file->fd);
     rrd_close(rrd_file);
 
     free(pdp_new);
@@ -1549,9 +1548,9 @@ int _rrd_update(
   err_free_updvals:
     free(updvals);
   err_close:
-    rrd_free(&rrd);
-    close(rrd_file->fd);
     rrd_close(rrd_file);
+  err_free:
+    rrd_free(&rrd);
   err_out:
     return (-1);
 }