[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[lvm-devel] [PATCH 1/1] Proposal patch to fix clvmd restart



Hi

Attached patch with some clvmd_restart fixes.

Zdenek
>From a9c47b41068eec94d9eebf0b8278a65fd482a282 Mon Sep 17 00:00:00 2001
Message-Id: <a9c47b41068eec94d9eebf0b8278a65fd482a282 1315562312 git zkabelac redhat com>
In-Reply-To: <cover 1315562312 git zkabelac redhat com>
References: <cover 1315562312 git zkabelac redhat com>
From: Zdenek Kabelac <zkabelac redhat com>
Date: Fri, 9 Sep 2011 11:16:03 +0200
Subject: [PATCH 1/1] Proposal patch to fix clvmd restart

Since I'm not clvmd expert - this patch is only proposal -

Original this patch tried to fix Clang warning about possible access
via lv_name NULL pointer.

However a bit more closer code inspection seems to reveal potentional
bug in the loop gathering '-E' options - it looks like it skips every
second lv_name as it calls get_next_excl_lock() twice in the loop.

So this patch removes this double call.

Also replaces allocation of memory (strdup) with just pointer assigment
(since we are going to call execve anyway).

Also check for  !lv_name only when lv_name is defined
(and as I'm not quite sure what state this really is - putting a FIXME
arround - since this rather looks like some expected situation ??)

Signed-off-by: Zdenek Kabelac <zkabelac redhat com>
---
 daemons/clvmd/clvmd-command.c |   45 ++++++++++++++++++----------------------
 1 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/daemons/clvmd/clvmd-command.c b/daemons/clvmd/clvmd-command.c
index 75f1367..597fb6e 100644
--- a/daemons/clvmd/clvmd-command.c
+++ b/daemons/clvmd/clvmd-command.c
@@ -362,39 +362,39 @@ void cmd_client_cleanup(struct local_client *client)
 static int restart_clvmd(void)
 {
 	char **argv = NULL;
-	char *debug_arg = NULL, *lv_name;
-	int i, argc = 0, max_locks = 0;
+	char *lv_name;
+	int argc = 0, max_locks = 0;
 	struct dm_hash_node *hn = NULL;
+	char debug_arg[16];
+	char e_arg[] = "-E";
+	char clvmd_arg[] = "clvmd";
 
 	DEBUGLOG("clvmd restart requested\n");
 
 	/* Count exclusively-open LVs */
-	hn = NULL;
 	do {
 		hn = get_next_excl_lock(hn, &lv_name);
-		if (lv_name)
+		if (lv_name) {
 			max_locks++;
-	} while (hn && *lv_name);
+			if (!*lv_name)
+				break; /* FIXME: Is this error ? */
+		}
+	} while (hn);
 
 	/* clvmd + locks (-E uuid) + debug (-d X) + NULL */
-	argv = malloc((max_locks * 2 + 4) * sizeof(*argv));
-	if (!argv)
+	if (!(argv = malloc((max_locks * 2 + 4) * sizeof(*argv))))
 		goto_out;
 
 	/*
 	 * Build the command-line
 	 */
-	argv[argc++] = strdup("clvmd");
-	if (!argv[0])
-		goto_out;
+	argv[argc++] = clvmd_arg;
 
 	/* Propogate debug options */
 	if (clvmd_get_debug()) {
-		if (!(debug_arg = malloc(16)) ||
-		    dm_snprintf(debug_arg, 16, "-d%u", clvmd_get_debug()) < 0)
+		if (dm_snprintf(debug_arg, sizeof(debug_arg), "-d%u", clvmd_get_debug()) < 0)
 			goto_out;
 		argv[argc++] = debug_arg;
-		debug_arg = NULL;
 	}
 
 	/*
@@ -403,34 +403,29 @@ static int restart_clvmd(void)
 	 */
 
 	/* Now add the exclusively-open LVs */
+	hn = NULL;
 	do {
 		hn = get_next_excl_lock(hn, &lv_name);
 		if (lv_name) {
-			argv[argc] = strdup("-E");
-			if (!argv[argc++])
-				goto_out;
-			argv[argc] = strdup(lv_name);
-			if (!argv[argc++])
-				goto_out;
-
+			if (!*lv_name)
+				break; /* FIXME: Is this error ? */
+			argv[argc++] = e_arg;
+			argv[argc++] = lv_name;
 			DEBUGLOG("excl lock: %s\n", lv_name);
-			hn = get_next_excl_lock(hn, &lv_name);
 		}
-	} while (hn && *lv_name);
+	} while (hn);
 	argv[argc++] = NULL;
 
 	/* Exec new clvmd */
 	DEBUGLOG("--- Restarting %s ---\n", CLVMD_PATH);
+
 	/* NOTE: This will fail when downgrading! */
 	execve(CLVMD_PATH, argv, NULL);
 out:
 	/* We failed */
 	DEBUGLOG("Restart of clvmd failed.\n");
 
-	for (i = 0; i < argc && argv[i]; i++)
-		free(argv[i]);
 	free(argv);
-	free(debug_arg);
 
 	return EIO;
 }
-- 
1.7.6.1


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]