summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorKrishnan Parthasarathi <kparthas@redhat.com>2013-02-27 17:55:47 +0530
committerAnand Avati <avati@redhat.com>2013-03-25 14:50:53 -0700
commit87d03fa7f48af6500cb8277db96ee7f6c690ea1c (patch)
treee258152c3b91c09b908dd14a77aba68d0e1f4a08
parent544945a128b4de9c6b767939fb4c4c216b095d23 (diff)
glusterd: Removed fd leaks in glusterfs_start utility function
PROBLEM: The FILE* associated with the pidfile was leaked if pmap_registry_search on the brickinfo' path failed. FIX: Eliminates the use of the FILE* that was leaked. Uses glusterd_is_service_running utility function in place of the earlier attempt to check for the same. Change-Id: I94082bd5a94b8a6340f8cc11726d3264e364efe6 BUG: 916549 Signed-off-by: Krishnan Parthasarathi <kparthas@redhat.com> Reviewed-on: http://review.gluster.org/4596 Tested-by: Gluster Build System <jenkins@build.gluster.com> Reviewed-by: Jeff Darcy <jdarcy@redhat.com> Reviewed-by: Anand Avati <avati@redhat.com>
-rwxr-xr-xtests/bugs/bug-916549.t19
-rw-r--r--tests/include.rc6
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-log-ops.c6
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-replace-brick.c7
-rw-r--r--xlators/mgmt/glusterd/src/glusterd-utils.c171
-rw-r--r--xlators/mgmt/glusterd/src/glusterd.h14
6 files changed, 104 insertions, 119 deletions
diff --git a/tests/bugs/bug-916549.t b/tests/bugs/bug-916549.t
new file mode 100755
index 00000000000..d6a45b8270f
--- /dev/null
+++ b/tests/bugs/bug-916549.t
@@ -0,0 +1,19 @@
+#!/bin/bash
+
+. $(dirname $0)/../include.rc
+
+cleanup;
+
+TEST glusterd;
+TEST $CLI volume create $V0 $H0:$B0/${V0}1;
+TEST $CLI volume start $V0;
+
+pid_file=$(ls /var/lib/glusterd/vols/$V0/run);
+brick_pid=$(cat /var/lib/glusterd/vols/$V0/run/$pid_file);
+
+
+kill -SIGKILL $brick_pid;
+TEST $CLI volume start $V0 force;
+TEST process_leak_count $(pidof glusterd);
+
+cleanup;
diff --git a/tests/include.rc b/tests/include.rc
index 03c615e5e1a..4008ad36f0c 100644
--- a/tests/include.rc
+++ b/tests/include.rc
@@ -214,6 +214,12 @@ function build_tester ()
gcc -g -o $(dirname $cfile)/$execname $cfile
}
+function process_leak_count ()
+{
+ local pid=$1;
+ return $(ls -lh /proc/$pid/fd | grep "(deleted)"| wc -l)
+}
+
alias EXPECT='_EXPECT $LINENO'
alias TEST='_TEST $LINENO'
alias EXPECT_WITHIN='_EXPECT_WITHIN $LINENO'
diff --git a/xlators/mgmt/glusterd/src/glusterd-log-ops.c b/xlators/mgmt/glusterd/src/glusterd-log-ops.c
index bd07af5b00d..0136cddc9ad 100644
--- a/xlators/mgmt/glusterd/src/glusterd-log-ops.c
+++ b/xlators/mgmt/glusterd/src/glusterd-log-ops.c
@@ -160,7 +160,6 @@ glusterd_op_log_rotate (dict_t *dict)
xlator_t *this = NULL;
char *volname = NULL;
char *brick = NULL;
- char path[PATH_MAX] = {0,};
char logfile[PATH_MAX] = {0,};
char pidfile[PATH_MAX] = {0,};
FILE *file = NULL;
@@ -216,10 +215,7 @@ cont:
valid_brick = 1;
- GLUSTERD_GET_VOLUME_DIR (path, volinfo, priv);
- GLUSTERD_GET_BRICK_PIDFILE (pidfile, path, brickinfo->hostname,
- brickinfo->path);
-
+ GLUSTERD_GET_BRICK_PIDFILE (pidfile, volinfo, brickinfo, priv);
file = fopen (pidfile, "r+");
if (!file) {
gf_log ("", GF_LOG_ERROR, "Unable to open pidfile: %s",
diff --git a/xlators/mgmt/glusterd/src/glusterd-replace-brick.c b/xlators/mgmt/glusterd/src/glusterd-replace-brick.c
index ec69b363763..36f0b10dc6f 100644
--- a/xlators/mgmt/glusterd/src/glusterd-replace-brick.c
+++ b/xlators/mgmt/glusterd/src/glusterd-replace-brick.c
@@ -210,7 +210,6 @@ glusterd_op_stage_replace_brick (dict_t *dict, char **op_errstr,
dict_t *ctx = NULL;
glusterd_conf_t *priv = NULL;
char *savetok = NULL;
- char voldir[PATH_MAX] = {0};
char pidfile[PATH_MAX] = {0};
char *task_id_str = NULL;
xlator_t *this = NULL;
@@ -443,10 +442,8 @@ glusterd_op_stage_replace_brick (dict_t *dict, char **op_errstr,
}
}
- GLUSTERD_GET_VOLUME_DIR (voldir, volinfo, priv);
- GLUSTERD_GET_BRICK_PIDFILE (pidfile, voldir,
- src_brickinfo->hostname,
- src_brickinfo->path);
+ GLUSTERD_GET_BRICK_PIDFILE (pidfile, volinfo, src_brickinfo,
+ priv);
if ((replace_op != GF_REPLACE_OP_COMMIT_FORCE) &&
!glusterd_is_service_running (pidfile, NULL)) {
snprintf(msg, sizeof(msg), "Source brick %s:%s "
diff --git a/xlators/mgmt/glusterd/src/glusterd-utils.c b/xlators/mgmt/glusterd/src/glusterd-utils.c
index b5e9a746ad2..9eff3929431 100644
--- a/xlators/mgmt/glusterd/src/glusterd-utils.c
+++ b/xlators/mgmt/glusterd/src/glusterd-utils.c
@@ -1183,6 +1183,35 @@ out:
return ret;
}
+/* Caller should ensure that brick process is not running*/
+static void
+_reap_brick_process (char *pidfile, char *brickpath)
+{
+ unlink (pidfile);
+ /* Brick process is not running and pmap may have an entry for it.*/
+ pmap_registry_remove (THIS, 0, brickpath,
+ GF_PMAP_PORT_BRICKSERVER, NULL);
+}
+
+static int
+_mk_rundir_p (glusterd_volinfo_t *volinfo)
+{
+ char voldir[PATH_MAX] = {0,};
+ char rundir[PATH_MAX] = {0,};
+ glusterd_conf_t *priv = NULL;
+ xlator_t *this = NULL;
+ int ret = -1;
+
+ this = THIS;
+ priv = this->private;
+ GLUSTERD_GET_VOLUME_DIR (voldir, volinfo, priv);
+ snprintf (rundir, sizeof (rundir)-1, "%s/run", voldir);
+ ret = mkdir_p (rundir, 0777, _gf_true);
+ if (ret)
+ gf_log (this->name, GF_LOG_ERROR, "Failed to create rundir");
+ return ret;
+}
+
int32_t
glusterd_volume_start_glusterfs (glusterd_volinfo_t *volinfo,
glusterd_brickinfo_t *brickinfo,
@@ -1191,17 +1220,13 @@ glusterd_volume_start_glusterfs (glusterd_volinfo_t *volinfo,
int32_t ret = -1;
xlator_t *this = NULL;
glusterd_conf_t *priv = NULL;
- char pidfile[PATH_MAX] = {0,};
+ char pidfile[PATH_MAX+1] = {0,};
char volfile[PATH_MAX] = {0,};
- char path[PATH_MAX] = {0,};
runner_t runner = {0,};
- char rundir[PATH_MAX] = {0,};
char exp_path[PATH_MAX] = {0,};
char logfile[PATH_MAX] = {0,};
int port = 0;
int rdma_port = 0;
- FILE *file = NULL;
- gf_boolean_t is_locked = _gf_false;
char socketpath[PATH_MAX] = {0};
char glusterd_uuid[1024] = {0,};
#ifdef DEBUG
@@ -1216,100 +1241,59 @@ glusterd_volume_start_glusterfs (glusterd_volinfo_t *volinfo,
priv = this->private;
GF_ASSERT (priv);
- GLUSTERD_GET_VOLUME_DIR (path, volinfo, priv);
- snprintf (rundir, PATH_MAX, "%s/run", path);
- ret = mkdir (rundir, 0777);
-
- if ((ret == -1) && (EEXIST != errno)) {
- gf_log (this->name, GF_LOG_ERROR, "Unable to create rundir %s."
- "Reason : %s", rundir, strerror (errno));
+ ret = _mk_rundir_p (volinfo);
+ if (ret)
goto out;
- }
-
- glusterd_set_brick_socket_filepath (volinfo, brickinfo, socketpath,
- sizeof (socketpath));
- GLUSTERD_GET_BRICK_PIDFILE (pidfile, path, brickinfo->hostname,
- brickinfo->path);
-
- file = fopen (pidfile, "r+");
- if (file) {
- ret = lockf (fileno (file), F_TLOCK, 0);
- if (ret && ((EAGAIN == errno) || (EACCES == errno))) {
- ret = 0;
- gf_log (this->name, GF_LOG_DEBUG, "brick %s:%s "
- "already started", brickinfo->hostname,
- brickinfo->path);
- goto connect;
- }
- }
-
- ret = pmap_registry_search (this, brickinfo->path,
- GF_PMAP_PORT_BRICKSERVER);
- if (ret) {
- ret = 0;
- file = fopen (pidfile, "r+");
- if (file) {
- ret = lockf (fileno (file), F_TLOCK, 0);
- if (ret && ((EAGAIN == errno) || (EACCES == errno))) {
- ret = 0;
- gf_log (this->name, GF_LOG_DEBUG, "brick %s:%s "
- "already started", brickinfo->hostname,
- brickinfo->path);
- goto connect;
- } else if (0 == ret) {
- is_locked = _gf_true;
- }
- }
- /* This means, pmap has the entry, remove it */
- ret = pmap_registry_remove (this, 0, brickinfo->path,
- GF_PMAP_PORT_BRICKSERVER, NULL);
- }
- unlink (pidfile);
+ GLUSTERD_GET_BRICK_PIDFILE (pidfile, volinfo, brickinfo, priv);
+ if (glusterd_is_service_running (pidfile, NULL))
+ goto connect;
- gf_log (this->name, GF_LOG_DEBUG, "About to start glusterfs"
- " for brick %s:%s", brickinfo->hostname,
- brickinfo->path);
- GLUSTERD_REMOVE_SLASH_FROM_PATH (brickinfo->path, exp_path);
- snprintf (volfile, PATH_MAX, "%s.%s.%s", volinfo->volname,
- brickinfo->hostname, exp_path);
-
- if (!brickinfo->logfile && volinfo->logdir) {
- snprintf (logfile, PATH_MAX, "%s/%s.log", volinfo->logdir,
- exp_path);
- brickinfo->logfile = gf_strdup (logfile);
- } else if (!brickinfo->logfile) {
- snprintf (logfile, PATH_MAX, "%s/bricks/%s.log",
- DEFAULT_LOG_FILE_DIRECTORY, exp_path);
- brickinfo->logfile = gf_strdup (logfile);
- }
+ _reap_brick_process (pidfile, brickinfo->path);
port = brickinfo->port;
if (!port)
port = pmap_registry_alloc (THIS);
runinit (&runner);
-
#ifdef DEBUG
if (priv->valgrind) {
+ /* Run bricks with valgrind */
if (volinfo->logdir) {
snprintf (valgrind_logfile, PATH_MAX,
- "%s/valgrind-%s-%s.log", volinfo->logdir,
+ "%s/valgrind-%s-%s.log",
+ volinfo->logdir,
volinfo->volname, exp_path);
} else {
- snprintf (valgrind_logfile, PATH_MAX,
- "%s/bricks/valgrind-%s-%s.log",
- DEFAULT_LOG_FILE_DIRECTORY,
- volinfo->volname, exp_path);
+ snprintf (valgrind_logfile, PATH_MAX,
+ "%s/bricks/valgrind-%s-%s.log",
+ DEFAULT_LOG_FILE_DIRECTORY,
+ volinfo->volname, exp_path);
}
- /* Run bricks with valgrind */
+
runner_add_args (&runner, "valgrind", "--leak-check=full",
"--trace-children=yes", NULL);
runner_argprintf (&runner, "--log-file=%s", valgrind_logfile);
- }
+ }
#endif
+ GLUSTERD_REMOVE_SLASH_FROM_PATH (brickinfo->path, exp_path);
+ snprintf (volfile, PATH_MAX, "%s.%s.%s", volinfo->volname,
+ brickinfo->hostname, exp_path);
+
+ if (volinfo->logdir) {
+ snprintf (logfile, PATH_MAX, "%s/%s.log",
+ volinfo->logdir, exp_path);
+ } else {
+ snprintf (logfile, PATH_MAX, "%s/bricks/%s.log",
+ DEFAULT_LOG_FILE_DIRECTORY, exp_path);
+ }
+ if (!brickinfo->logfile)
+ brickinfo->logfile = gf_strdup (logfile);
+
+ glusterd_set_brick_socket_filepath (volinfo, brickinfo, socketpath,
+ sizeof (socketpath));
(void) snprintf (glusterd_uuid, 1024, "*-posix.glusterd-uuid=%s",
uuid_utoa (MY_UUID));
- runner_add_args (&runner, SBIN_DIR"/glusterfsd",
+ runner_add_args (&runner, SBIN_DIR"/glusterfsd",
"-s", brickinfo->hostname, "--volfile-id", volfile,
"-p", pidfile, "-S", socketpath,
"--brick-name", brickinfo->path,
@@ -1317,7 +1301,7 @@ glusterd_volume_start_glusterfs (glusterd_volinfo_t *volinfo,
"--xlator-option", glusterd_uuid,
NULL);
- runner_add_arg (&runner, "--brick-port");
+ runner_add_arg (&runner, "--brick-port");
if (volinfo->transport_type != GF_TRANSPORT_BOTH_TCP_RDMA) {
runner_argprintf (&runner, "%d", port);
} else {
@@ -1346,7 +1330,6 @@ glusterd_volume_start_glusterfs (glusterd_volinfo_t *volinfo,
if (ret)
goto out;
- //pmap_registry_bind (THIS, port, brickinfo->path);
brickinfo->port = port;
brickinfo->rdma_port = rdma_port;
@@ -1355,13 +1338,6 @@ connect:
if (ret)
goto out;
out:
- if (is_locked && file)
- if (lockf (fileno (file), F_ULOCK, 0) < 0)
- gf_log (this->name, GF_LOG_WARNING, "Cannot unlock "
- "pidfile: %s reason: %s", pidfile,
- strerror(errno));
- if (file)
- fclose (file);
return ret;
}
@@ -1419,7 +1395,6 @@ glusterd_volume_stop_glusterfs (glusterd_volinfo_t *volinfo,
xlator_t *this = NULL;
glusterd_conf_t *priv = NULL;
char pidfile[PATH_MAX] = {0,};
- char path[PATH_MAX] = {0,};
int ret = 0;
GF_ASSERT (volinfo);
@@ -1434,11 +1409,7 @@ glusterd_volume_stop_glusterfs (glusterd_volinfo_t *volinfo,
if (GLUSTERD_STATUS_STARTED == volinfo->status) {
(void) glusterd_brick_disconnect (brickinfo);
-
- GLUSTERD_GET_VOLUME_DIR (path, volinfo, priv);
- GLUSTERD_GET_BRICK_PIDFILE (pidfile, path, brickinfo->hostname,
- brickinfo->path);
-
+ GLUSTERD_GET_BRICK_PIDFILE (pidfile, volinfo, brickinfo, priv);
ret = glusterd_service_stop ("brick", pidfile, SIGTERM, _gf_false);
if (ret == 0) {
glusterd_set_brick_status (brickinfo, GF_BRICK_STOPPED);
@@ -4483,7 +4454,6 @@ glusterd_add_brick_to_dict (glusterd_volinfo_t *volinfo,
char key[1024] = {0};
char base_key[1024] = {0};
char pidfile[PATH_MAX] = {0};
- char path[PATH_MAX] = {0};
xlator_t *this = NULL;
glusterd_conf_t *priv = NULL;
@@ -4516,9 +4486,7 @@ glusterd_add_brick_to_dict (glusterd_volinfo_t *volinfo,
if (ret)
goto out;
- GLUSTERD_GET_VOLUME_DIR (path, volinfo, priv);
- GLUSTERD_GET_BRICK_PIDFILE (pidfile, path, brickinfo->hostname,
- brickinfo->path);
+ GLUSTERD_GET_BRICK_PIDFILE (pidfile, volinfo, brickinfo, priv);
brick_online = glusterd_is_service_running (pidfile, &pid);
@@ -5684,7 +5652,6 @@ glusterd_brick_statedump (glusterd_volinfo_t *volinfo,
xlator_t *this = NULL;
glusterd_conf_t *conf = NULL;
char pidfile_path[PATH_MAX] = {0,};
- char path[PATH_MAX] = {0,};
char dumpoptions_path[PATH_MAX] = {0,};
FILE *pidfile = NULL;
pid_t pid = -1;
@@ -5709,9 +5676,7 @@ glusterd_brick_statedump (glusterd_volinfo_t *volinfo,
goto out;
}
- GLUSTERD_GET_VOLUME_DIR (path, volinfo, conf);
- GLUSTERD_GET_BRICK_PIDFILE (pidfile_path, path, brickinfo->hostname,
- brickinfo->path);
+ GLUSTERD_GET_BRICK_PIDFILE (pidfile_path, volinfo, brickinfo, conf);
pidfile = fopen (pidfile_path, "r");
if (!pidfile) {
@@ -7368,8 +7333,8 @@ glusterd_is_same_address (char *name1, char *name2)
{
struct addrinfo *addr1 = NULL;
struct addrinfo *addr2 = NULL;
- struct addrinfo *p = NULL;
- struct addrinfo *q = NULL;
+ struct addrinfo *p = NULL;
+ struct addrinfo *q = NULL;
gf_boolean_t ret = _gf_false;
int gai_err = 0;
diff --git a/xlators/mgmt/glusterd/src/glusterd.h b/xlators/mgmt/glusterd/src/glusterd.h
index 34593202b07..a1327e60c12 100644
--- a/xlators/mgmt/glusterd/src/glusterd.h
+++ b/xlators/mgmt/glusterd/src/glusterd.h
@@ -373,12 +373,14 @@ typedef ssize_t (*gd_serialize_t) (struct iovec outmsg, void *args);
} \
} while (0)
-#define GLUSTERD_GET_BRICK_PIDFILE(pidfile,volpath,hostname,brickpath) { \
- char exp_path[PATH_MAX] = {0,}; \
- GLUSTERD_REMOVE_SLASH_FROM_PATH (brickpath, exp_path); \
- snprintf (pidfile, PATH_MAX, "%s/run/%s-%s.pid", \
- volpath, hostname, exp_path); \
- }
+#define GLUSTERD_GET_BRICK_PIDFILE(pidfile,volinfo,brickinfo, priv) do { \
+ char exp_path[PATH_MAX] = {0,}; \
+ char volpath[PATH_MAX] = {0,}; \
+ GLUSTERD_GET_VOLUME_DIR (volpath, volinfo, priv); \
+ GLUSTERD_REMOVE_SLASH_FROM_PATH (brickinfo->path, exp_path); \
+ snprintf (pidfile, PATH_MAX, "%s/run/%s-%s.pid", \
+ volpath, brickinfo->hostname, exp_path); \
+ } while (0)
#define GLUSTERD_GET_NFS_PIDFILE(pidfile,nfspath) { \
snprintf (pidfile, PATH_MAX, "%s/run/nfs.pid", \