glusterfsd, libgfapi: destroy the temporary graphs constructed for comparison

* The new and the oldgraphs which have been constructed whenever there is
  a volfile change (either reconfigure of the existing graph or creating
  a new graph) for comparison should be freed. Otherwise frequent graph
  changes will lead to huge memory leak

Change-Id: I4faddb1aa9393b34cd2de6732e537a60f600026a
BUG: 948178
Signed-off-by: Raghavendra Bhat <raghavendra@redhat.com>
Reviewed-on: http://review.gluster.org/5388
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Anand Avati <avati@redhat.com>

6 files changed, 223 insertions, 355 deletions

diff --git a/api/src/glfs-mgmt.c b/api/src/glfs-mgmt.c
index 20362543e..6843e9cb3 100644
--- a/api/src/glfs-mgmt.c
+++ b/api/src/glfs-mgmt.c
@@ -203,162 +203,6 @@ out:
 
 
 static int
-xlator_equal_rec (xlator_t *xl1, xlator_t *xl2)
-{
-	xlator_list_t *trav1 = NULL;
-	xlator_list_t *trav2 = NULL;
-	int	       ret   = 0;
-
-	if (xl1 == NULL || xl2 == NULL) {
-		gf_log ("xlator", GF_LOG_DEBUG, "invalid argument");
-		return -1;
-	}
-
-	trav1 = xl1->children;
-	trav2 = xl2->children;
-
-	while (trav1 && trav2) {
-		ret = xlator_equal_rec (trav1->xlator, trav2->xlator);
-		if (ret) {
-			gf_log ("glfs-mgmt", GF_LOG_DEBUG,
-				"xlators children not equal");
-			goto out;
-		}
-
-		trav1 = trav1->next;
-		trav2 = trav2->next;
-	}
-
-	if (trav1 || trav2) {
-		ret = -1;
-		goto out;
-	}
-
-	if (strcmp (xl1->name, xl2->name)) {
-		ret = -1;
-		goto out;
-	}
-out :
-	return ret;
-}
-
-
-static gf_boolean_t
-is_graph_topology_equal (glusterfs_graph_t *graph1,
-			 glusterfs_graph_t *graph2)
-{
-	xlator_t    *trav1    = NULL;
-	xlator_t    *trav2    = NULL;
-	gf_boolean_t ret      = _gf_true;
-
-	trav1 = graph1->first;
-	trav2 = graph2->first;
-
-	ret = xlator_equal_rec (trav1, trav2);
-
-	if (ret) {
-		gf_log ("glfs-mgmt", GF_LOG_DEBUG,
-			"graphs are not equal");
-		ret = _gf_false;
-		goto out;
-	}
-
-	ret = _gf_true;
-	gf_log ("glfs-mgmt", GF_LOG_DEBUG,
-		"graphs are equal");
-
-out:
-	return ret;
-}
-
-
-/* Function has 3types of return value 0, -ve , 1
- *   return 0	       =======> reconfiguration of options has succeeded
- *   return 1	       =======> the graph has to be reconstructed and all the xlators should be inited
- *   return -1(or -ve) =======> Some Internal Error occurred during the operation
- */
-static int
-glusterfs_volfile_reconfigure (struct glfs *fs, FILE *newvolfile_fp)
-{
-	glusterfs_graph_t *oldvolfile_graph = NULL;
-	glusterfs_graph_t *newvolfile_graph = NULL;
-	FILE		  *oldvolfile_fp    = NULL;
-	glusterfs_ctx_t	  *ctx		    = NULL;
-
-	int ret = -1;
-
-	oldvolfile_fp = tmpfile ();
-	if (!oldvolfile_fp)
-		goto out;
-
-	if (!fs->oldvollen) {
-		ret = 1; // Has to call INIT for the whole graph
-		goto out;
-	}
-	fwrite (fs->oldvolfile, fs->oldvollen, 1, oldvolfile_fp);
-	fflush (oldvolfile_fp);
-	if (ferror (oldvolfile_fp)) {
-		goto out;
-	}
-
-	oldvolfile_graph = glusterfs_graph_construct (oldvolfile_fp);
-	if (!oldvolfile_graph) {
-		goto out;
-	}
-
-	newvolfile_graph = glusterfs_graph_construct (newvolfile_fp);
-	if (!newvolfile_graph) {
-		goto out;
-	}
-
-	if (!is_graph_topology_equal (oldvolfile_graph,
-				      newvolfile_graph)) {
-
-		ret = 1;
-		gf_log ("glfs-mgmt", GF_LOG_DEBUG,
-			"Graph topology not equal(should call INIT)");
-		goto out;
-	}
-
-	gf_log ("glfs-mgmt", GF_LOG_DEBUG,
-		"Only options have changed in the new "
-		"graph");
-
-	ctx = fs->ctx;
-
-	if (!ctx) {
-		gf_log ("glfs-mgmt", GF_LOG_ERROR,
-			"glusterfs_ctx_get() returned NULL");
-		goto out;
-	}
-
-	oldvolfile_graph = ctx->active;
-
-	if (!oldvolfile_graph) {
-		gf_log ("glfs-mgmt", GF_LOG_ERROR,
-			"glusterfs_ctx->active is NULL");
-		goto out;
-	}
-
-	/* */
-	ret = glusterfs_graph_reconfigure (oldvolfile_graph,
-					   newvolfile_graph);
-	if (ret) {
-		gf_log ("glfs-mgmt", GF_LOG_DEBUG,
-			"Could not reconfigure new options in old graph");
-		goto out;
-	}
-
-	ret = 0;
-out:
-	if (oldvolfile_fp)
-		fclose (oldvolfile_fp);
-
-	return ret;
-}
-
-
-static int
 glusterfs_oldvolfile_update (struct glfs *fs, char *volfile, ssize_t size)
 {
 	int ret = -1;
@@ -451,7 +295,8 @@ mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
 	*  return -1(or -ve) =======> Some Internal Error occurred during the operation
 	*/
 
-	ret = glusterfs_volfile_reconfigure (fs, tmpfp);
+	ret = glusterfs_volfile_reconfigure (fs->oldvollen, tmpfp, fs->ctx,
+                                             fs->oldvolfile);
 	if (ret == 0) {
 		gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
 			"No need to re-load volfile, reconfigure done");
diff --git a/glusterfsd/src/glusterfsd-mgmt.c b/glusterfsd/src/glusterfsd-mgmt.c
index e40d19b08..071066bdb 100644
--- a/glusterfsd/src/glusterfsd-mgmt.c
+++ b/glusterfsd/src/glusterfsd-mgmt.c
@@ -1330,187 +1330,7 @@ out:
 static char *oldvolfile = NULL;
 static int oldvollen = 0;
 
-static int
-xlator_equal_rec (xlator_t *xl1, xlator_t *xl2)
-{
-        xlator_list_t *trav1 = NULL;
-        xlator_list_t *trav2 = NULL;
-        int            ret   = 0;
-
-        if (xl1 == NULL || xl2 == NULL) {
-                gf_log ("xlator", GF_LOG_DEBUG, "invalid argument");
-                return -1;
-        }
-
-        trav1 = xl1->children;
-        trav2 = xl2->children;
-
-        while (trav1 && trav2) {
-                ret = xlator_equal_rec (trav1->xlator, trav2->xlator);
-                if (ret) {
-                        gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
-                                "xlators children not equal");
-                        goto out;
-                }
-
-                trav1 = trav1->next;
-                trav2 = trav2->next;
-        }
-
-        if (trav1 || trav2) {
-                ret = -1;
-                goto out;
-        }
-
-        if (strcmp (xl1->name, xl2->name)) {
-                ret = -1;
-                goto out;
-        }
-
-	/* type could have changed even if xlator names match,
-	   e.g cluster/distrubte and cluster/nufa share the same
-	   xlator name
-	*/
-        if (strcmp (xl1->type, xl2->type)) {
-                ret = -1;
-                goto out;
-        }
-out :
-        return ret;
-}
-
-static gf_boolean_t
-is_graph_topology_equal (glusterfs_graph_t *graph1,
-                                glusterfs_graph_t *graph2)
-{
-        xlator_t    *trav1    = NULL;
-        xlator_t    *trav2    = NULL;
-        gf_boolean_t ret      = _gf_true;
-
-        trav1 = graph1->first;
-        trav2 = graph2->first;
-
-        ret = xlator_equal_rec (trav1, trav2);
-
-        if (ret) {
-                gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
-                        "graphs are not equal");
-                ret = _gf_false;
-                goto out;
-        }
-
-        ret = _gf_true;
-        gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
-                "graphs are equal");
-
-out:
-        return ret;
-}
-
-/* Function has 3types of return value 0, -ve , 1
- *   return 0          =======> reconfiguration of options has succeeded
- *   return 1          =======> the graph has to be reconstructed and all the xlators should be inited
- *   return -1(or -ve) =======> Some Internal Error occurred during the operation
- */
-static int
-glusterfs_volfile_reconfigure (FILE *newvolfile_fp)
-{
-        glusterfs_graph_t *oldvolfile_graph = NULL;
-        glusterfs_graph_t *newvolfile_graph = NULL;
-        int                oldvolfile_fd    = -1;
-        FILE              *oldvolfile_fp    = NULL;
-        glusterfs_ctx_t   *ctx              = NULL;
-        char               template[PATH_MAX] = {0};
-
-        int ret = -1;
-
-        strcpy (template, "/tmp/tmp.XXXXXX");
-        oldvolfile_fd = mkstemp (template);
-        if (oldvolfile_fd == -1) {
-                gf_log ("glusterfsd-mgmt", GF_LOG_ERROR, "Unable to create "
-                        "temporary file: %s (%s)", template,
-                        strerror (errno));
-                goto out;
-        }
-
-        ret = unlink (template);
-        if (ret < 0) {
-                gf_log ("glusterfsd-mgmt", GF_LOG_WARNING, "Unable to delete "
-                        "file: %s", template);
-        }
-
-        oldvolfile_fp = fdopen (oldvolfile_fd, "w+b");
-        if (!oldvolfile_fp) {
-                gf_log ("glusterfsd-mgmt", GF_LOG_CRITICAL, "Failed to create "
-                        "temporary volfile");
-                goto out;
-        }
-
-        if (!oldvollen) {
-                ret = 1; // Has to call INIT for the whole graph
-                goto out;
-        }
-        fwrite (oldvolfile, oldvollen, 1, oldvolfile_fp);
-        fflush (oldvolfile_fp);
-        if (ferror (oldvolfile_fp)) {
-                goto out;
-        }
-
-
-        oldvolfile_graph = glusterfs_graph_construct (oldvolfile_fp);
-        if (!oldvolfile_graph) {
-                goto out;
-        }
-
-        newvolfile_graph = glusterfs_graph_construct (newvolfile_fp);
-        if (!newvolfile_graph) {
-                goto out;
-        }
-
-        if (!is_graph_topology_equal (oldvolfile_graph,
-                                      newvolfile_graph)) {
-
-                ret = 1;
-                gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
-                        "Graph topology not equal(should call INIT)");
-                goto out;
-        }
-
-        gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
-                "Only options have changed in the new "
-                "graph");
-
-        ctx = glusterfsd_ctx;
-
-        oldvolfile_graph = ctx->active;
 
-        if (!oldvolfile_graph) {
-                gf_log ("glusterfsd-mgmt", GF_LOG_ERROR,
-                        "glusterfs_ctx->active is NULL");
-                goto out;
-        }
-
-        /* */
-        ret = glusterfs_graph_reconfigure (oldvolfile_graph,
-                                           newvolfile_graph);
-        if (ret) {
-                gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
-                        "Could not reconfigure new options in old graph");
-                goto out;
-        }
-
-        ret = 0;
-out:
-        if (oldvolfile_fp) {
-                fclose (oldvolfile_fp);
-
-        } else if (-1 != oldvolfile_fd) {
-            close (oldvolfile_fd);
-
-        }
-
-        return ret;
-}
 
 int
 mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
@@ -1576,7 +1396,7 @@ mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
         *  return -1(or -ve) =======> Some Internal Error occurred during the operation
         */
 
-        ret = glusterfs_volfile_reconfigure (tmpfp);
+        ret = glusterfs_volfile_reconfigure (oldvollen, tmpfp, ctx, oldvolfile);
         if (ret == 0) {
                 gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
                         "No need to re-load volfile, reconfigure done");
@@ -1609,6 +1429,7 @@ mgmt_getspec_cbk (struct rpc_req *req, struct iovec *iov, int count,
                 volfilebuf = GF_REALLOC (oldvolfile, size);
         else
                 volfilebuf = GF_CALLOC (1, size, gf_common_mt_char);
+
         if (!volfilebuf) {
                 ret = -1;
                 goto out;
diff --git a/libglusterfs/src/graph.c b/libglusterfs/src/graph.c
index 1dba63fc0..e76df1ca5 100644
--- a/libglusterfs/src/graph.c
+++ b/libglusterfs/src/graph.c
@@ -537,6 +537,184 @@ glusterfs_graph_activate (glusterfs_graph_t *graph, glusterfs_ctx_t *ctx)
 
 
 int
+xlator_equal_rec (xlator_t *xl1, xlator_t *xl2)
+{
+        xlator_list_t *trav1 = NULL;
+        xlator_list_t *trav2 = NULL;
+        int            ret   = 0;
+
+        if (xl1 == NULL || xl2 == NULL) {
+                gf_log ("xlator", GF_LOG_DEBUG, "invalid argument");
+                return -1;
+        }
+
+        trav1 = xl1->children;
+        trav2 = xl2->children;
+
+        while (trav1 && trav2) {
+                ret = xlator_equal_rec (trav1->xlator, trav2->xlator);
+                if (ret) {
+                        gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
+                                "xlators children not equal");
+                        goto out;
+                }
+
+                trav1 = trav1->next;
+                trav2 = trav2->next;
+        }
+
+        if (trav1 || trav2) {
+                ret = -1;
+                goto out;
+        }
+
+        if (strcmp (xl1->name, xl2->name)) {
+                ret = -1;
+                goto out;
+        }
+
+	/* type could have changed even if xlator names match,
+	   e.g cluster/distrubte and cluster/nufa share the same
+	   xlator name
+	*/
+        if (strcmp (xl1->type, xl2->type)) {
+                ret = -1;
+                goto out;
+        }
+out :
+        return ret;
+}
+
+
+gf_boolean_t
+is_graph_topology_equal (glusterfs_graph_t *graph1, glusterfs_graph_t *graph2)
+{
+        xlator_t    *trav1    = NULL;
+        xlator_t    *trav2    = NULL;
+        gf_boolean_t ret      = _gf_true;
+
+        trav1 = graph1->first;
+        trav2 = graph2->first;
+
+        ret = xlator_equal_rec (trav1, trav2);
+
+        if (ret) {
+                gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
+                        "graphs are not equal");
+                ret = _gf_false;
+                goto out;
+        }
+
+        ret = _gf_true;
+        gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
+                "graphs are equal");
+
+out:
+        return ret;
+}
+
+
+/* Function has 3types of return value 0, -ve , 1
+ *   return 0          =======> reconfiguration of options has succeeded
+ *   return 1          =======> the graph has to be reconstructed and all the xlators should be inited
+ *   return -1(or -ve) =======> Some Internal Error occurred during the operation
+ */
+int
+glusterfs_volfile_reconfigure (int oldvollen, FILE *newvolfile_fp,
+                               glusterfs_ctx_t *ctx, const char *oldvolfile)
+{
+        glusterfs_graph_t *oldvolfile_graph = NULL;
+        glusterfs_graph_t *newvolfile_graph = NULL;
+        FILE              *oldvolfile_fp    = NULL;
+        gf_boolean_t      active_graph_found = _gf_true;
+
+        int ret = -1;
+
+        if (!oldvollen) {
+                ret = 1; // Has to call INIT for the whole graph
+                goto out;
+        }
+
+        if (!ctx) {
+                gf_log ("glusterfsd-mgmt", GF_LOG_ERROR,
+			"ctx is NULL");
+		goto out;
+	}
+
+        oldvolfile_graph = ctx->active;
+        if (!oldvolfile_graph) {
+                active_graph_found = _gf_false;
+                gf_log ("glusterfsd-mgmt", GF_LOG_ERROR,
+                        "glusterfs_ctx->active is NULL");
+
+                oldvolfile_fp = tmpfile ();
+                if (!oldvolfile_fp) {
+                        gf_log ("glusterfsd-mgmt", GF_LOG_ERROR, "Unable to "
+                                "create temporary volfile: (%s)",
+                                strerror (errno));
+                        goto out;
+                }
+
+                fwrite (oldvolfile, oldvollen, 1, oldvolfile_fp);
+                fflush (oldvolfile_fp);
+                if (ferror (oldvolfile_fp)) {
+                        goto out;
+                }
+
+                oldvolfile_graph = glusterfs_graph_construct (oldvolfile_fp);
+                if (!oldvolfile_graph)
+                        goto out;
+        }
+
+        newvolfile_graph = glusterfs_graph_construct (newvolfile_fp);
+        if (!newvolfile_graph) {
+                goto out;
+        }
+
+        if (!is_graph_topology_equal (oldvolfile_graph,
+                                      newvolfile_graph)) {
+
+                ret = 1;
+                gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
+                        "Graph topology not equal(should call INIT)");
+                goto out;
+        }
+
+        gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
+                "Only options have changed in the new "
+                "graph");
+
+        /* */
+        ret = glusterfs_graph_reconfigure (oldvolfile_graph,
+                                           newvolfile_graph);
+        if (ret) {
+                gf_log ("glusterfsd-mgmt", GF_LOG_DEBUG,
+                        "Could not reconfigure new options in old graph");
+                goto out;
+        }
+
+        ret = 0;
+out:
+        if (oldvolfile_fp)
+                fclose (oldvolfile_fp);
+
+        /*  Do not simply destroy the old graph here. If the oldgraph
+            is constructed here in this function itself instead of getting
+            it from ctx->active (which happens only of ctx->active is NULL),
+            then destroy the old graph. If some i/o is still happening in
+            the old graph and the old graph is obtained from ctx->active,
+            then destroying the graph will cause problems.
+        */
+        if (!active_graph_found && oldvolfile_graph)
+                glusterfs_graph_destroy (oldvolfile_graph);
+        if (newvolfile_graph)
+                glusterfs_graph_destroy (newvolfile_graph);
+
+        return ret;
+}
+
+
+int
 glusterfs_graph_reconfigure (glusterfs_graph_t *oldgraph,
                              glusterfs_graph_t *newgraph)
 {
@@ -562,5 +740,12 @@ glusterfs_graph_reconfigure (glusterfs_graph_t *oldgraph,
 int
 glusterfs_graph_destroy (glusterfs_graph_t *graph)
 {
+        xlator_tree_free (graph->first);
+
+        if (graph) {
+                list_del_init (&graph->list);
+                GF_FREE (graph);
+        }
+
         return 0;
 }
diff --git a/libglusterfs/src/graph.y b/libglusterfs/src/graph.y
index ca4301ff0..a220abeb9 100644
--- a/libglusterfs/src/graph.y
+++ b/libglusterfs/src/graph.y
@@ -482,6 +482,7 @@ preprocess (FILE *srcfp, FILE *dstfp)
 					cmd_buf_size *= 2;
 					cmd = GF_REALLOC (cmd, cmd_buf_size);
                                         if (cmd == NULL) {
+                                                GF_FREE (result);
                                                 return -1;
                                         }
 
@@ -523,6 +524,7 @@ preprocess (FILE *srcfp, FILE *dstfp)
 out:
 	fseek (srcfp, 0L, SEEK_SET);
 	fseek (dstfp, 0L, SEEK_SET);
+
 	GF_FREE (cmd);
 	GF_FREE (result);
 
diff --git a/libglusterfs/src/xlator.c b/libglusterfs/src/xlator.c
index a7caedf02..d02947550 100644
--- a/libglusterfs/src/xlator.c
+++ b/libglusterfs/src/xlator.c
@@ -538,10 +538,26 @@ out:
         return;
 }
 
+int
+xlator_list_destroy (xlator_list_t *list)
+{
+        xlator_list_t *next = NULL;
+
+        while (list) {
+                next = list->next;
+                GF_FREE (list);
+                list = next;
+        }
+
+        return 0;
+}
+
 
 int
 xlator_tree_free (xlator_t *tree)
 {
+        volume_opt_list_t *vol_opt = NULL;
+        volume_opt_list_t *tmp     = NULL;
         xlator_t *trav = tree;
         xlator_t *prev = tree;
 
@@ -552,9 +568,19 @@ xlator_tree_free (xlator_t *tree)
 
         while (prev) {
                 trav = prev->next;
-                dict_destroy (prev->options);
+                if (prev->dlhandle)
+                        dlclose (prev->dlhandle);
+                dict_unref (prev->options);
                 GF_FREE (prev->name);
                 GF_FREE (prev->type);
+                xlator_list_destroy (prev->children);
+                xlator_list_destroy (prev->parents);
+
+                list_for_each_entry_safe (vol_opt, tmp, &prev->volume_options,
+                                          list) {
+                        list_del_init (&vol_opt->list);
+                        GF_FREE (vol_opt);
+                }
                 GF_FREE (prev);
                 prev = trav;
         }
@@ -696,21 +722,6 @@ loc_is_root (loc_t *loc)
 }
 
 int
-xlator_list_destroy (xlator_list_t *list)
-{
-        xlator_list_t *next = NULL;
-
-        while (list) {
-                next = list->next;
-                GF_FREE (list);
-                list = next;
-        }
-
-        return 0;
-}
-
-
-int
 xlator_destroy (xlator_t *xl)
 {
         volume_opt_list_t *vol_opt = NULL;
diff --git a/libglusterfs/src/xlator.h b/libglusterfs/src/xlator.h
index d23ee0c25..84a028fbc 100644
--- a/libglusterfs/src/xlator.h
+++ b/libglusterfs/src/xlator.h
@@ -932,5 +932,9 @@ enum gf_hdsk_event_notify_op {
         GF_EN_DEFRAG_STATUS,
         GF_EN_MAX,
 };
-
+gf_boolean_t
+is_graph_topology_equal (glusterfs_graph_t *graph1, glusterfs_graph_t *graph2);
+int
+glusterfs_volfile_reconfigure (int oldvollen, FILE *newvolfile_fp,
+                               glusterfs_ctx_t *ctx, const char *oldvolfile);
 #endif /* _XLATOR_H */